Consolidate definitions of fields, objects, transactions, and features #5122
Conversation
|
This is a useful change and would make the manual process of updating third party libraries with new field types, transactions and ledger entries a little easier. |
|
Yes, that would be great, but the preprocessor cannot parse JSON or CSV or any other format. It would require the introduction of an additional build step to generate these headers from that definition file. I would welcome that contribution from someone else, but it's out of scope for my current work. This was a drive-by change I made while working on single asset vault. |
|
Thanks for the reply. I do think the pool of people who could wrangle the rippled build process to add a code generation stage is probably limited to one :-) If it's not something that appeals, fair enough. |
There was a problem hiding this comment.
Few initial comments, also this: I wonder if this pattern is perhaps too permissive
#pragma push_macro("FOO")
#undef FOO
#pragma push_macro("BAR")
#undef BAR
#define FOO . . .;
#define BAR . . .;
#include <xrpl/protocol/. . .>
#undef FOO
#pragma pop_macro("FOO")
#undef BAR
#pragma pop_macro("BAR")Could this be better (i.e. more robust) ?
#if defined(FOO) || defined(BAR)
#error "Macro FOO or BAR was found to be unexpectedly defined"
#endif
#define FOO . . .;
#define BAR . . . ;
#include <xrpl/protocol/. . .>
#undef FOO
#undef BAR| ltORACLE = 0x0080, | ||
| #pragma push_macro("OBJECT") | ||
| #undef OBJECT | ||
| #define OBJECT(tag, value, name, fields) tag = value, |
There was a problem hiding this comment.
For consistency, please follow the same style as in Feature.h (vertical spacing)
There was a problem hiding this comment.
Would LEDGER_ENTRY be a more specific name for the macro? OBJECT seems very general and suggests it can be used elsewhere.
include/xrpl/protocol/features.h
Outdated
There was a problem hiding this comment.
I would put it in include/xrpl/protocol/detail . Also, how about name which clearly refers to a header it is used in, e.g. Feature_list.h or even better Feature.inl ?
There was a problem hiding this comment.
I much prefer the simple features.h naming.
There was a problem hiding this comment.
I totally do not mind .ipp, or even stick with .h if you insist, but then I would very much prefer if it was moved to detail subdir as not to make clutter. These files are not standalone, like all well-written header files should be, so having a different extension is a hint to the user that IDE errors are expected when viewing them
There was a problem hiding this comment.
never mind, I see they have been moved to detail, thanks !
There was a problem hiding this comment.
I moved them to detail/ subdirectory.
@donovanhide does the |
|
@mvadari Thanks for pointing it out. It is interesting and I was unaware of it, but it is not the full answer. I need to know all the fields for each ledger entry and transaction type, but that seems to be missing from that response. |
thejohnfreeman
requested review from
bthomee
and removed request for
ximinez
Note that the internal structure of fields defined with CONSTRUCT_UNTYPED_SFIELD, such as the sfMemo field, for example, cannot be known (as before). |
|
Just to be clear of my use case, it is updating the data structures here: https://github.com/rubblelabs/ripple/blob/master/data/ledgerentry.go And the canonical source of truth for these data structures, encodings and flags are the source code files being changed in this PR. It would be much more efficient if my library and others could be derived from an easily machine readable file rather than a hard to diff C++ file. Seeing as this is the essence of the Ripple protocol, it doesn't seem like an unreasonable request :-) |
@donovanhide that's a fair statement, but I think it should be in an RPC rather than in the source code (perhaps added to |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #5122 +/- ##
=========================================
- Coverage 76.2% 76.2% -0.0%
=========================================
Files 760 762 +2
Lines 61568 61478 -90
Branches 8126 8121 -5
=========================================
- Hits 46898 46816 -82
+ Misses 14670 14662 -8
... and 4 files with indirect coverage changes |
There was a problem hiding this comment.
I like this idea of changing just one file a lot but IMHO there are two inconvenient issues:
-
Because of the macros, some IDE (I have CLion) are not going to be able to resolve the references. For instance, clicking on a field name in transactions.m or ledger_entries.m doesn't navigate to the fields declaration. And searching for the field with the
sfprefix is not going to be helpful either - see the next issue. -
I'd prefer that the fields keep
sfprefix. For instance, when I search forsfAmountI can see the field type and the occurrences. But try to search forAmountand you'll get huge number of hits. Of course I know where to find the field declaration but I have to navigate to this file separately.
| @@ -0,0 +1,112 @@ | |||
| //------------------------------------------------------------------------------ | |||
There was a problem hiding this comment.
Should the .m files be .h? It's confusing since .m is usually associated with Objective-C.
There was a problem hiding this comment.
I asked to use a different extension that cannot be confused with a header, because these files are not headers in normal sense of the word (because of macros they rely on) - they cannot be included as-is, they won't parse in an IDE, they cannot be formatted with clang-format. I suggested .inl as that's typically used for "inline files" but @thejohnfreeman chose different.
There was a problem hiding this comment.
It's confusing IMO, but I'm fine with this.
There was a problem hiding this comment.
This : https://gcc.gnu.org/onlinedocs/cpp/Header-Files.html suggests headers can be used for macros.
There was a problem hiding this comment.
Yes, headers are often used for macro definitions. Also the language attaches no meaning to file extension. However things get a little tricky for IDEs and other tools when, what appears to be a header file from extension, does not work like one. In this case the apparent header files rely on a macro definitions brought from the outside without the relevant #include inside the file, which is not normal practice of how header files work.
If that helps to get the PR approved, I am happy for @thejohnfreeman to rename these files to any extension, including .h. I guess it does not matter much.
There was a problem hiding this comment.
.m feels odd but not a showstopper I guess. I'm more concerned about two other issues that I mentioned.
There was a problem hiding this comment.
Would everyone like .macro more than both .m and .h?
There was a problem hiding this comment.
If for some reason .h is not an option then .macro is better.
But as I mentioned earlier, using macros in general prevents IDE (at least CLion) resolving the fields reference. I find this really inconvenient.
| //============================================================================== | ||
|
|
||
| #if !defined(LEDGER_ENTRY) | ||
| #error "undefined macro: LEDGER_ENTRY" |
There was a problem hiding this comment.
I just occurred to me that in order to make this file (and others) similar to a regular header, we could do this instead of #error :
#define LEDGER_ENTRY(...)
#pragma message("using dummy LEDGER_ENTRY")
Note the dummy macro is empty. This in turn would make the file well-behaved in IDEs and other tools, meaning it could be renamed back to .h if you so desire.
There was a problem hiding this comment.
@gregtatcam any comments on this ?
There was a problem hiding this comment.
It still doesn't solve the issue of resolving the fields reference by IDE, does it?
There was a problem hiding this comment.
You are right - it does not. An actual macro doing something could do that, maybe ?
Leverages a preprocessor technique to put the entire definitions of fields, objects, transactions, and features into one file for each type. When you want to add a field, object, transaction, or feature, you touch just one file. When you want to inspect a definition, you look up just one file.