Use compile-time constants instead of unnamed enumerations (remove "enum hack") #10749

pull practicalswift wants to merge 1 commits into bitcoin:master from practicalswift:enum-hack changing 5 files +14 −20

practicalswift commented at 2:51 PM on July 5, 2017: contributor

Use compile-time constants instead of unnamed enumerations (remove "enum hack").
fanquake added the label Refactoring on Jul 5, 2017
jonasschnelli commented at 4:13 PM on July 5, 2017: contributor

utACK 0f3d58be40672151468700fb5a05898468814880
jgarzik commented at 5:43 PM on July 5, 2017: contributor

This enum technique is a well-defined way to generate typed constants.

Is it causing problems somewhere?
TheBlueMatt commented at 6:22 PM on July 5, 2017: member

utACK 0f3d58be40672151468700fb5a05898468814880
gmaxwell approved
gmaxwell commented at 12:53 AM on July 6, 2017: contributor

ACK.
benma commented at 11:44 AM on July 25, 2017: none

First time I read about constexpr, and I also googled for enum vs constexpr. For what it's worth, I've seen quite a few threads that lean towards enums even with constexpr being available, like https://stackoverflow.com/a/37259949.

constexpr is easier to read, and it doesn't seem to have a downside at least in this PR.
practicalswift commented at 11:50 AM on July 25, 2017: contributor

@benma I take that as an utACK? :-)
Use compile-time constants instead of unnamed enumerations (remove "enum hack") 1e65f0f339
in src/consensus/consensus.h:33 in 0f3d58be40 outdated
```
  30 | -    /* Interpret sequence numbers as relative lock-time constraints. */
  31 | -    LOCKTIME_VERIFY_SEQUENCE = (1 << 0),
  32 | -
  33 | -    /* Use GetMedianTimePast() instead of nTime for end point timestamp. */
  34 | -    LOCKTIME_MEDIAN_TIME_PAST = (1 << 1),
  35 | -};
```
benma commented at 4:11 PM on July 25, 2017:

Isn't an enum appropriate when it's indeed an enumeration?

Aso, please rebase, as there are other consts that could be constexprs in consensus.h now:

static const size_t MIN_TRANSACTION_WEIGHT = WITNESS_SCALE_FACTOR * 60; // 60 is the lower bound for the size of a valid serialized CTransaction static const size_t MIN_SERIALIZABLE_TRANSACTION_WEIGHT = WITNESS_SCALE_FACTOR * 10; // 10 is the lower bound for the size of a serialized CTransaction

Still a bit confused about the whole thing, seems like simple const would also suffice. Is the advantage of constexpr just that it gives a compile time error in case the expression can't be evaluated at compile time?
practicalswift commented at 10:30 PM on July 25, 2017:

Sure, that case could be made. I can do it as a scoped named enum (enum class). Any suggestion on a proper name for that enumeration?

The goal of this PR is to get rid of enum hacks. Changing existing const:s to constexpr:s is not within that scope :-)

When introducing new compile-time constants I prefer constexpr which is explicit about the intention of the code (getting a compile-time constant). Do you see any compelling reasons to avoid using constexpr (which implies const) for compile-time constants?
benma commented at 2:52 PM on July 26, 2017:

Thanks. I just thew in some thoughts in case you can pick something from it. I have no preference either way.
practicalswift force-pushed on Jul 25, 2017
practicalswift commented at 10:15 PM on July 25, 2017: contributor

Rebased!
benma commented at 2:51 PM on July 26, 2017: none

utACK 1e65f0f3396d5d7eaa8e8dd3668dfa180b541c18
jnewbery commented at 4:33 PM on August 17, 2017: member

ACK 1e65f0f3396d5d7eaa8e8dd3668dfa180b541c18