It would be desirable to keep enums and string representations in sync that way, but it's more world-reorganisation than I feel this patch immediately demands.

To make things a bit better I've changed the array into a switch statement so that the order of the enumeration elements no longer matters, removed flags will cause an error and added flags will at least fall back on printing their int representations.

I'd prefer to leave it as is, but just put a static_assert in the code that at least fails compilation if something is added in one but not both places.

To match the static_assert in arch/arm/faults.hh, presumably.  Any thoughts, Nilay?

The python Enum class (in src/python/m5/params.py) does what you want, I think.  See, for example, OpClass in src/cpu/FuncUnit.py, which generates build/*/enums/OpClass.{cc,hh}.  This was originally designed to support enums being used as SimObject parameters, but there's no reason it has to be limited to that.

I've put the static_assert in as suggested.  Using Enum would significantly complicate using StaticInst::Flags, not least because it changes the scoping of the enumeration elements (assuming that it's used without modification).

It would be nice if the python code had more flexibility to deal with different scopes, but it seems like you could get around the issue with a 'using' declaration, right?

Actually a typedef would probably make more sense... define the enum as StaticInstFlags, then inside StaticInst put:

typedef Enums::StaticInstFlags Flags;

Yeah, that would work.  Adding an option to allowing a namespace to be specified so the enum elements didn't have to occupy the same namespace as all other enums would make it better.

I think the extra complexity from adding another auto generated dependency makes the idea much less attractive though.
 

Yes, the ability to specify a different namespace (or elide it altogether) is what I meant when I said "It would be nice if the python code had more flexibility".  If you'd like to add that, please do, but I was trying to put forward a simpler solution.

I don't see how yet another dependency makes this less attractive... we already have dozens, if not hundreds, so one more is really in the noise.  All you have to do is '#include "enums/StaticInstFlags.hh"'  in static_inst.hh and scons should take care of everything else.

Yeah, I was reacting to your "flexibility" comment, Steve.  The 'namespace' thing was a just a suggestion for that ;)

There are a lot of dependencies, yes, but this one feels like it compromises the grep-ability of the uncompiled codebase as most of the other dependencies for static_inst.hh are hidden behind common names in TheISA and similar structures.

I suppose there is a downside in terms of spreading things across more files, but I think the automated generation of the enum and the strings from a single list outweighs that, and I think it would be hard to automate this process without having the enum in a separate file.  Basically Nilay wished for a tool like this, and I'm pointing out that we already have one, and it seems odd not to use it.

In particular, a big problem here seems to be that you weren't aware of this facility and thus went off and had to spend time coming up with your own solution.  While that's unfortunate, I'm concerned that putting in the manual solution you have here will cause other developers to believe that this is the best we can do, and then they'll also replicate this unnecessary effort because they still won't be aware that there's a more automated mechanism available.

If there are some small tweaks that would make it more palatable, let's talk.  I actually don't think that the namespace tweaks you mentioned would be that hard.  Then again, I'm not sure they address your objection either... I think all it would do would be to let you get rid of the typedef and move the #include of the enum definition inside the class StaticInst declaration, and that's not a very big change (and I'm not even sure it's an improvement).

Or we could just rewrite everything in Ada... the one positive memory I have of that language is that you could get at the string representations of enums for free.

Another tool that you can use is protobuf (which I believe is already a dependency).  You can create enums in a .proto file and protoc will generate the machinery that you guys are talking about.

At the moment protobuf is optional (perhaps we want to reconsider this decision in the future).

A possible compromise solution would be to use a set of macros.  Something like:

<In some header file>
#define STRINGIFY(str) #str
#define IDENTITY(str) str

<In static_inst.hh>
#define STATIC_INST_FLAGS_BUILDER(elem) \
    elem(IsLoad), /*!< comment */ \
    elem(IsStore) /*!< comment */

class StaticInst {
    enum Flags {
        STATIC_INST_FLAGS_BUILDER(IDENTITY)
    };
    static const char *flagsNames[];
};

<In static_inst.cc>
static const char *StaticInst::flagsNames[] = { STATIC_INST_FLAGS_BUILDER(STRINGIFY) };

Yes, it's an ugly macro, but it is a solution in (ahem) pure inline-with-the-file-body C++.
The only other annoying thing about this solution is that the element Doxygen comments will get lost in the macro expansion.  I've played with doxygen a bit to see what I could do about it (IANA-Doxygen-Fan) and failed to find a way to put them back in.

Anyway.  All that being said.  I'm going to leave this patch on the board for a while for further comments (and probably until the patch with the callsite that's using it is on the review board). 

Personally I think the gain of having the enum values in the same .hh file with the StaticInst object is very small relative to the clunkiness and redundancy of the solution.  Of course, that's purely subjective, which is what keeps the discussion lively.

If you're using grep to find things, you could just grep the .py files too, and the enum will be easy to find.  Alternatively, if you're using something more sophisticated like cscope, then perhaps we should find a way of indexing some of the generated files as well.

Though finding things in gem5 can be tricky, I'd argue that if you're digging in to the StaticInst object and looking at the instruction flags, you either already know that there's a ton of auto-generated code in the ISA decoder, or you're about to find that out.  Thus the innocence you're trying to preserve by keeping this particular enum "pure" is either long gone or about to be harshly violated anyway ;).

Yeah.  They're all good points.  I'll have a go at a scons-generated Enum version of a patch.

I'm honestly not sure.  I converted a lot of stuff in that changeset to use the new enum stuff and it could be that the structs didn't seem to buy much.  They just meant that you have to have longer names.  You can't have a using directive on a struct, so you always had to write Foo::Bar and you could never say Bar.  Why not just say Foo_Bar if you need to disambiguate.  All that said, it may have been a mistake to do that.

If you do want better scoping, don't use the struct stuff from before (i.e. don't do "struct Foo { enum { BAR }};") since it really is just a longer name.  C++11 has a better way: strongly typed enumerations (a.k.a "enum struct"). Where you'd say "enum struct Foo { BAR };".  Then each enum is a different type and only its enum values can be used.

http://en.wikipedia.org/wiki/C++11#Strongly_typed_enumerations
http://en.cppreference.com/w/cpp/language/enum. (the stuff about scoped enumerations)

Thanks, Nate... the strongly typed enum stuff is new to me.  While the stronger type checking is nice, I don't see how it helps the naming issue.  According to http://en.cppreference.com/w/cpp/language/using_declaration, "Only the name explicitly mentioned in the using-declaration is transferred into the declarative scope: in particular, enumerators are not transferred when the enumeration type name is using-declared."

In contrast, Andrew figured out how to import the struct-declared flags into a class scope by using the struct as a base, something that doesn't appear to work with the "enum struct" definition.