Calculate numFeatures automatically #5324
Conversation
- Update documentation
ximinez
force-pushed
the
ximinez/numFeatures
branch
from
412f6c3 to
16d1e3a
Compare
ximinez
changed the title
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5324 +/- ##
=========================================
- Coverage 78.2% 78.1% -0.1%
=========================================
Files 790 790
Lines 67741 67908 +167
Branches 8178 8229 +51
=========================================
+ Hits 52969 53023 +54
- Misses 14771 14885 +114
+ Partials 1 0 -1
... and 18 files with indirect coverage changes |
include/xrpl/protocol/Feature.h
Outdated
| @@ -80,7 +80,27 @@ namespace detail { | |||
| // Feature.cpp. Because it's only used to reserve storage, and determine how | |||
| // large to make the FeatureBitset, it MAY be larger. It MUST NOT be less than | |||
| // the actual number of amendments. A LogicError on startup will verify this. | |||
| static constexpr std::size_t numFeatures = 88; | |||
| static constexpr std::size_t numFeatures = 0 | |||
There was a problem hiding this comment.
This is so simple, it's brilliant, but I would prefer to further improve readability, by:
- moving all the preprocessor (except
#includeobviously) outside of0 ... ;block, i.e. one part immediately before and one part immediately after. - put the
0 ... ;block in parentheses - squint your eyes a little and see how close this resembles something like
constexpr numFeatures = (
0
# define ... + 1
# define ... + 1
# define ... + 1
# include ...
);There was a problem hiding this comment.
... or actually, because 0 + + 1 is valid and identical to 0 + 1, do this
constexpr numFeatures = (
0 +
#include ...
);There was a problem hiding this comment.
- moving all the preprocessor (except
#includeobviously) outside of0 ... ;block, i.e. one part immediately before and one part immediately after.
This is a great suggestion! I made this change, and added the parenthesis, and the +.
Do you want to re-review?
There was a problem hiding this comment.
This is so simple, it's brilliant
BTW, thank you!
|
I was interested in looking into existing solutions like this and stumbled on https://www.boost.org/doc/libs/1_87_0/libs/preprocessor/doc/ref/counter.html I tried to integrate it into XRPL_FEATURE/FIX macros, but seems like passing |
I think what @ximinez proposed here is better because it's easier to understand. |
ximinez
added
the
Ready to merge
include/xrpl/protocol/Feature.h
Outdated
| * SUPPORTED, change the macro parameter in features.macro to Supported::yes. | ||
| * | ||
| * 4) When the feature is ready to be ENABLED, change the macro parameter in | ||
| * features.macro parameter to `VoteBehavior::DefaultYes`. |
There was a problem hiding this comment.
Given that the section below explicitly says that newly supported amendments are NOT DefaultYes (which is indeed the policy), would it make sense to remove this point ?
There was a problem hiding this comment.
Fixed and reworded.
Bronek
removed
the
Ready to merge
- Reword some of the instructions to better reflect current behaviors.
ximinez
force-pushed
the
ximinez/numFeatures
branch
from
d6f8855 to
0fc28f1
Compare
Fix typo found by @mvadari Co-authored-by: Mayukha Vadari <[email protected]>
| * a `VoteBehavior::DefaultNo` indefinitely so that external governance can | ||
| * make the decision on when to activate it. High priority bug fixes can be | ||
| * an exception to this rule. In such cases, change the macro parameter in | ||
| * features.macro to `VoteBehavior::DefaultYes`. |
There was a problem hiding this comment.
perhaps add something like ... and ensure that the fix has been clearly communicated to the community, using the appropriate channels. . Or even swap the order i.e. ensure the communication in the first place, then change the macro parameter.
I know this goes implied, but we must assume that not everyone reading this comment will grasp the significance of clear communication in this case.
| static constexpr std::size_t numFeatures = | ||
| (0 + | ||
| #include <xrpl/protocol/detail/features.macro> | ||
| ); |
High Level Overview of Change
Finally, we are free from the tyranny of needing to update
numFeaturesinFeature.h!This PR, if merged, will take advantage of the
XRPL_FEATUREandXRPL_FIXmacros, and add a newXRPL_RETIREto automatically setnumFeaturesso it can be used elsewhere.I want to call this PR "Trivial", but the macros are just fiddly enough that I'd rather have two eyes on it.
Context of Change
Requiring manual updates of
numFeaturesis an annoying manual process that is easily forgotten, and leads to frequent merge conflicts.Type of Change
Before / After
If relevant, use this section for an English description of the change at a technical level.
If this change affects an API, examples should be included here.
For performance-impacting changes, please provide these details:
-->
Test Plan
As long as rippled builds and is able to start, this change is good.
Future Tasks
It would be cool to take advantage of
features.macroinside of theFeatureCollectionsctor so that the object can beconstafter construction. Each feature could be a public member variable ofFeatureCollections, and the global variables can simply be a reference to the corresponding members.