Prefer enum over static const names?

In symrec.h I found the following code:

    // generic variable
    static const unsigned int local = 1;

    // formal parameter
    static const unsigned int formal = 2;

    // this symbol may NOT become a variable.
    // (symbol added to a static workspace)
    static const unsigned int added_static = 4;

    // this symbol was recognized as a variable from syntax
    static const unsigned int variable = 8;

These are class static variables, but it is clear that they aren’t really variables (i.e., modifiable) at all and that the intention is to replace C-style #define of constants. For example, here is a member function that uses these static variables:

      bool is_local (void) const
      {
        return m_storage_class & local;
      }

Wouldn’t it be better from a C++ style perspective to use an enum here and capitalize the constants?

For example,

enum symrec_t : unsigned int { LOCAL = 1, FORMAL = 2, ADDED_STATIC = 4, VARIABLE = 8 };

Of course, maybe this might just get you in to a nest of silly compiler warnings about conversions between unsigned int and symrec_t. I worked up a quick patch, but it doesn’t fully compile (enum.patch (3.0 KB)).

Is it worth trying to be more C++ here, or just leave as is? Even if we don’t implement enum I think these are compile-time constants that probably deserve capitalization.

1 Like

Yes, I like the idea with the enum :slightly_smiling_face: :+1: What belongs together is not in one place only. Can you keep the comments about the semantic of those enumerators in your changed code?

Yes, that’s not a problem. Comments can go anywhere in C++ code.

It turns out that this is a typical C++ problem. If you want to be utterly correct and typesafe it means writing tons and tons of code. In this case, enum doesn’t get you full safety because enum values implicitly convert to int so it is possible to write

symrec_t what_does_this_mean_var = LOCAL * FORMAL;

To be truly correct one would need to use enum class from C++11. However, then you’re just generating a lot of code because, after defining the class, one would need to add operators for each and every permitted operation. One wouldn’t include an operator for *, but one would have to write functions for |,&,~,^,=|,=&,=^. It’s all a little crazy. For an excellent read on how to do this, see Using enum classes as type-safe bitmasks.

I’m not too concerned about an Octave programmer messing this up so I think either we move to enum or we use the bitset class from the STL. For documentation on bitset see bitset - C++ Reference.

Memory is really cheap these days so spending too much time optimizing this seems a little silly. However, I will point out that on most machines unsigned int is at least 4 bytes and we are only storing 4 values so we could just as well have defined m_storage_class as

 bool m_storage_class [4];

If we really want to save memory we should either use bitset which determines the optimal size at compile-time or change to using unsigned char for the enum type.

Do any Octave coders have a preference? And why?

The original idea was probably to use bitfields since these are flags that may be combined and because bitfields are what Real Programmers do. It doesn’t much matter to me now, but maybe we could defer these changes until a bit later. There are other things that we do in Octave that would be better handled by enums as well. For example, we have a number of functions that accept multiple logical option arguments where a single enum object would probably be better. That’s what Qt uses and I prefer that style over what we are doing with individual logical option arguments, but as you say, it requires more infrastructure. Qt has that, but for us to use it means either requiring some Qt classes in core Octave or providing our own classes to do this job.

BTW, in the particular case we are discussing here, I think we can (and should) eliminate the added_static flag completely and I’m looking at that now. I’ll also have to check the variable one because at first glance that seems kind of redundant now. We may only really need to know whether a symbol is a local variable or a function parameter.

EDIT: If you do make changes, it would be nice to make the names of the setter functions consistent. There are currently 4 properties and the setter functions are

      void mark_local (void)
      void mark_formal (void)
      void mark_added_static (void)
      void mark_as_variable (void)

Why should the last one be different? Seems like mark_variable would be a more consistent name.

It wasn’t much work, so I’m attaching a fully-baked changeset to convert to enum. symrec_enum.cset (4.8 KB)

If we don’t need certain fields then it would be just as easy to apply this and then remove the getter/setter functions associated with added_static that may no longer be relevant.

OK. I think it’s fine to commit this change.

Here is the documentation for the Qt class I was thinking of: QFlags Class | Qt Core 5.15.5

Done.

This looks very much like solution documented at wiggling-bits.net that I referenced above that use enum classes from C++11. As evidence, Qt really did just what I said would be necessary and wrote operator functions for all of the bitfield operations. Whew, that’s a lot of work. But as a class that could be extended it is true that they only had to do it once.