Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

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

Merged
merged 1 commit into from Nov 11, 2017
Merged

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

merged 1 commit into from Nov 11, 2017

Conversation

practicalswift
Copy link
Contributor

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

@jonasschnelli
Copy link
Contributor

utACK 0f3d58be40672151468700fb5a05898468814880

@jgarzik
Copy link
Contributor

jgarzik commented Jul 5, 2017

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

Is it causing problems somewhere?

@TheBlueMatt
Copy link
Contributor

utACK 0f3d58be40672151468700fb5a05898468814880

gmaxwell
gmaxwell approved these changes Jul 6, 2017
View reviewed changes
Copy link
Contributor

@gmaxwell gmaxwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK.

@benma
Copy link

benma commented Jul 25, 2017

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
Copy link
Contributor Author

@benma I take that as an utACK? :-)

benma
benma reviewed Jul 25, 2017
View reviewed changes
src/consensus/consensus.h

/* Use GetMedianTimePast() instead of nTime for end point timestamp. */
LOCKTIME_MEDIAN_TIME_PAST = (1 << 1),
};
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

  1. 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?
  2. The goal of this PR is to get rid of enum hacks. Changing existing const:s to constexpr:s is not within that scope :-)
  3. 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?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. I just thew in some thoughts in case you can pick something from it. I have no preference either way.

@practicalswift
Copy link
Contributor Author

Rebased!

@benma
Copy link

benma commented Jul 26, 2017

utACK 1e65f0f

@jnewbery
Copy link
Contributor

ACK 1e65f0f

@maflcko
Copy link
Member

maflcko commented Nov 11, 2017 

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA512

utACK 1e65f0f3396d5d7eaa8e8dd3668dfa180b541c18
-----BEGIN PGP SIGNATURE-----

iQIcBAEBCgAGBQJaBzQQAAoJENLqSFDnUosllrgP/0Jq2acX/JFfphTWDMcGzChi
NtMamQFeV1gvGLvUYW7UogT6R1BGCCCJ92OeVZFBQWsLo5ZkrucxQcUYKDnV5gKi
7LCl/8rz6LWmPvJszFPUjvE7/X5Nd6r1crCZJ9oPaO/w4tK8k5UeeI+ODn6+XUFN
WaoqP/sxnfeDm90fDybU7aix0yCaDsLZ/rHl6zVFSLQerb4McKG7ji/MjW4Bi/ii
wCAYHc9Gzfxa3y3AMaiA+zh/GMSLDBOvLaClWt3UlXI9pfVA9giP+PPgtKAHkz82
2lX+byUyGVK3n5oSBeJD8A4mmcOMvF0DT+6XlvlETX2ApnzZlkzX7tqtDa0w3UtJ
Ni1iCzZl11tgqXRTpQYaxTL5YdoDbeudhvz5gmZ+N2wNLIVcmbJSztt62eQI00GW
G5LPnWrDtaDOKn7Rv6kVJkgAcel9nIrIibvosQi0anrwVGZ3Fg7GXKY3kEhgwIuO
2YmjXJ5xEhwUDktopn7/Mt/fSeMWeg0Gg7oqGiJ6WCY0nYC9gHPJBd8k9nH/pOCP
V61mWhpDRjp+bQZvS6IL0GkQEJ/IPxPgTE4xNrsYMpVgBBPiWhjCy6VnAPbxZHd9
xJVpjJC+QKwR+zfVpoF2VzWKFlf28cfNSNh9z+8q6Juvv6maYUxlPxQ4zgMms8LA
1zrcJiqm/1ySme7GeWhb
=a+3Y
-----END PGP SIGNATURE-----

@maflcko maflcko merged commit 1e65f0f into bitcoin:master Nov 11, 2017
maflcko pushed a commit that referenced this pull request Nov 11, 2017
Merge #10749: Use compile-time constants instead of unnamed enumerati…
2adbddb 
…ons (remove "enum hack")

1e65f0f Use compile-time constants instead of unnamed enumerations (remove "enum hack") (practicalswift)

Pull request description:

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

Tree-SHA512: 1b6ebb2755398c5ebab6cce125b1dfc39cbd1504d98d55136b32703fe935c4070360ab3b2f52b1da48ba9f3b01082d204f3d87c92ccb5c8c333731f7f972e128
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 17, 2020
@PastaPastaPasta
Merge bitcoin#10749: Use compile-time constants instead of unnamed en…
2307488 
…umerations (remove "enum hack")

1e65f0f Use compile-time constants instead of unnamed enumerations (remove "enum hack") (practicalswift)

Pull request description:

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

Tree-SHA512: 1b6ebb2755398c5ebab6cce125b1dfc39cbd1504d98d55136b32703fe935c4070360ab3b2f52b1da48ba9f3b01082d204f3d87c92ccb5c8c333731f7f972e128
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
@PastaPastaPasta
Merge bitcoin#10749: Use compile-time constants instead of unnamed en…
f275b3f 
…umerations (remove "enum hack")

1e65f0f Use compile-time constants instead of unnamed enumerations (remove "enum hack") (practicalswift)

Pull request description:

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

Tree-SHA512: 1b6ebb2755398c5ebab6cce125b1dfc39cbd1504d98d55136b32703fe935c4070360ab3b2f52b1da48ba9f3b01082d204f3d87c92ccb5c8c333731f7f972e128
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 22, 2020
@PastaPastaPasta
Merge bitcoin#10749: Use compile-time constants instead of unnamed en…
aeaf3c5 
…umerations (remove "enum hack")

1e65f0f Use compile-time constants instead of unnamed enumerations (remove "enum hack") (practicalswift)

Pull request description:

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

Tree-SHA512: 1b6ebb2755398c5ebab6cce125b1dfc39cbd1504d98d55136b32703fe935c4070360ab3b2f52b1da48ba9f3b01082d204f3d87c92ccb5c8c333731f7f972e128
PastaPastaPasta pushed a commit to PastaPastaPasta/dash that referenced this pull request Jan 29, 2020
@PastaPastaPasta
Merge bitcoin#10749: Use compile-time constants instead of unnamed en…
3ab405a 
…umerations (remove "enum hack")

1e65f0f Use compile-time constants instead of unnamed enumerations (remove "enum hack") (practicalswift)

Pull request description:

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

Tree-SHA512: 1b6ebb2755398c5ebab6cce125b1dfc39cbd1504d98d55136b32703fe935c4070360ab3b2f52b1da48ba9f3b01082d204f3d87c92ccb5c8c333731f7f972e128
@practicalswift practicalswift deleted the enum-hack branch April 10, 2021 19:32
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Jun 26, 2021
Merge bitcoin#10749: Use compile-time constants instead of unnamed en…
1a88238 
…umerations (remove "enum hack")

1e65f0f Use compile-time constants instead of unnamed enumerations (remove "enum hack") (practicalswift)

Pull request description:

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

Tree-SHA512: 1b6ebb2755398c5ebab6cce125b1dfc39cbd1504d98d55136b32703fe935c4070360ab3b2f52b1da48ba9f3b01082d204f3d87c92ccb5c8c333731f7f972e128
@random-zebra random-zebra mentioned this pull request Jun 29, 2021
Merged
furszy added a commit to PIVX-Project/PIVX that referenced this pull request Jul 26, 2021
@furszy
Merge #2462: [Refactoring] Bitcoin 0.16 cherries
60d3629 
7a5d181 Use the character based overload for std::string::find. (Alin Rus)
c19401b Don't use pass by reference to const for cheaply-copied types (bool, char, etc.). (practicalswift)
4c5fe36 [Refactor] Remove unused fQuit var from checkqueue.h (donaloconnor)
fda7a5f Cleanup: remove unneeded header includes (random-zebra)
ac2476c Add test cases covering the relevant key length boundaries: 64 bytes +/- 1 byte for HMAC-SHA256 and 128 bytes +/- 1 byte for HMAC-SHA512 (practicalswift)
a346262 Fix code constness in CBlockIndex::GetAncestor() overloads (Dan Raviv)
65e3f4e Refactor: More range-based for loops (random-zebra)
dd3d3c4 Use range-based for loops (C++11) when looping over map elements (practicalswift)
5a7750a Move RPC registration out of AppInitParameterInteraction (Russell Yanofsky)
402b4c4 Use compile-time constants instead of unnamed enumerations (practicalswift)
bbac2d0 [Trivial] Fix indentation in coins.cpp (random-zebra)
e539c62 Small refactor of CCoinsViewCache::BatchWrite() (Dan Raviv)
ec91759 Use MakeUnique<T>(...) instead of std::unique_ptr<T>(new T(...)) (random-zebra)
93487b1 Use unique_ptr for pcoinscatcher/pcoinsdbview/pcoinsTip/pblocktree (random-zebra)
ff43d69 Use unique_ptr for pdbCopy (Db) and fix potential memory leak (practicalswift)
b4d9641 Use unique_ptr for dbenv (DbEnv) (practicalswift)
36108b9 Use unique_ptr for pfilter (CBloomFilter) (practicalswift)
ff1c454 Use unique_ptr for sem{Addnode,Outbound} (CSemaphore) (practicalswift)
93daf17 Use unique_ptr for httpRPCTimerInterface (HTTPRPCTimerInterface) (practicalswift)
020ac16 Init: Remove redundant exit(EXIT_FAILURE) instances and replace with (random-zebra)
b9f5d1f Remove duplicate uriParts.size() > 0 check (practicalswift)
440d961 Remove redundant check (!ecc is always true) (practicalswift)
bfd295b Remove redundant NULL checks after new (practicalswift)
97aad32 Make fUseCrypto atomic (MeshCollider)
2711f78 Consistent parameter names in txdb.h (MeshCollider)
d40df3a Fix race for mapBlockIndex in AppInitMain (random-zebra)
03b7766 Cleanup: remove unused functions to Hash the concat of 4 or more objects (random-zebra)
c520e0f Remove some unused functions and methods (Pieter Wuille)
508f1a1 range-based loops and const qualifications in net.cpp (Marko Bencun)
79b1e50 Refactor: implement CPubKey::data() (random-zebra)
614d26c Refactor: more &vec[0] to vec.data() (random-zebra)
02b6337 Changing &vec[0] to vec.data(), what 9804 missed (MeshCollider)
c1c8b05 Ensure that data types are consistent (jjz)
732bb9d Fix potential null dereferences (MeshCollider)
80f35f9 Remove unreachable code (practicalswift)

Pull request description:

  This is a collection of simple refactorings coming from upstream Bitcoin v0.16 (adapting/extending to PIVX-specific code where needed).

  Pull requests backported:

  - bitcoin#10845 (practicalswift)
  - bitcoin#11238 (MeshCollider)
  - bitcoin#11232 (jjz)
  - bitcoin#10793 (MeshCollider)
  - bitcoin#10888 (benma)
  - ~~bitcoin#11351 (danra)~~ [edit: removed - included in #2423]
  - bitcoin#11385 (sipa)
  - bitcoin#11107 (MeshCollider)
  - bitcoin#10898 (practicalswift)
  - bitcoin#11511 (donaloconnor)
  - bitcoin#11043 (practicalswift)
  - bitcoin#11353 (danra)
  - bitcoin#10749 (practicalswift)
  - bitcoin#11603 (ryanofsky)
  - bitcoin#10493 (practicalswift)
  - bitcoin#11337 (danra)
  - bitcoin#11516 (practicalswift)
  - bitcoin#10574 (practicalswift)
  - bitcoin#12108 (donaloconnor)
  - bitcoin#10839 (practicalswift)
  - bitcoin#12159 (kekimusmaximus)

ACKs for top commit:
  Fuzzbawls:
    ACK 7a5d181
  furszy:
    re-ACK 7a5d181 after rebase, no code changes. Merging..

Tree-SHA512: d92f5df47f443391a6531274a2efb9a4882c62d32eff628f795b3abce703f108c8b40ec80ac841cde6c5fdd5c9ff2b6056a31546ac2edda279f5f18fccc99c32
gades pushed a commit to cosanta/cosanta-core that referenced this pull request Feb 5, 2022
Merge bitcoin#10749: Use compile-time constants instead of unnamed en…
03df621 
…umerations (remove "enum hack")

1e65f0f Use compile-time constants instead of unnamed enumerations (remove "enum hack") (practicalswift)

Pull request description:

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

Tree-SHA512: 1b6ebb2755398c5ebab6cce125b1dfc39cbd1504d98d55136b32703fe935c4070360ab3b2f52b1da48ba9f3b01082d204f3d87c92ccb5c8c333731f7f972e128
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Aug 16, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@benma benma benma left review comments

@gmaxwell gmaxwell gmaxwell approved these changes

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@practicalswift @jonasschnelli @jgarzik @TheBlueMatt @benma @jnewbery @maflcko @gmaxwell @fanquake