Use macros to instantiate most SField instances:

[scottschurr]

Overview

There have been cases in the past where SFields have been defined in such a way that they did not follow our conventions. In particular, the string representation of an SField should align with the declared name of the global static const SField.

This commit leverages the preprocessor to encourage SFields to follow this naming convention. This should help reduce the likelihood of similar bugs in the future.

Details

This branch uses the token pasting capabilities of the preprocessor to assure that for any given SField the in-code name and the JSON text name match.

I also took the opportunity to use token pasting so the type of a TypedField always agrees with its SField::fieldType. In order to get the second change to work, I renamed the various TypedFields so their suffixes match the suffixes of the SerializedTypeID enum members.

While preparing for this change I noticed that the JSON text of the sfPayChannel SField was simply "Channel". That's the kind of mismatch this pull request intends to prevent. The JSON text is part of the API, so it cannot be changed. So this branch changes the name of the SField from sfPayChannel to sfChannel.

Type of Change

• Refactor (non-breaking change that only restructures code)

This change is internal only. There should be no need for a test plan, documentation changes, or release notes associated with this change.

[cjcobb23]

I like this change, for consistency reasons. My only issue is now that you can't find the definition of the various SField constants in the codebase. For example, there is no longer a definition for sfLedgerSequence that you can find via grep or something similar. It's not very difficult though to deduce that the constant is an SField and is probably defined in SField.cpp. Not sure how I feel about this. Otherwise, looks great.

[scottschurr]

Yeah, I agree with that drawback, @cjcobb23. Unfortunately I think the drawback is inevitable using the macro approach, since we want the macro to use name pasting to generate the name of the variable. That inevitably means the variable name in the declaration will be buried inside the macro magic and not grepable. I don't see any approach to the problem that can avoid this drawback. In the future metaprograming may be able to produce a text name given a declaration, but that's C++26 at the earliest.

If macros supported text extraction, then we could make this work. But that's not the universe we live in.

You can think about it and decide how the advantages and disadvantages balance out for you.

[pwang200]

Looks great! 👍

[pwang200]

@cjcobb23 Clion helped me. For this PR, it helped me comparing the preprocessor results with the previous code quickly.

FWIW, my flow has not changed much, i.e. one click to find the declaration and another to find the definition and the IDE helped to show the preprocessor result automatically.

[cjcobb23]

@pwang200 It's good to know that CLion was able to know what was happening. I'm fine with this as is. I think not being able to grep is a minor drawback, and the consistency gains are worth that drawback.

[scottschurr]

@cjcobb23 and @pwang200, it occurred to me that I can restructure the macros so the exact name of the SField is still findable by grepping. The revised macro relies of error checking, rather than correctness by construction, but it accomplishes the same end. The revised macro would result in declarations that look like this:

CONSTRUCT_TYPED_SFIELD(sfCloseResolution, "CloseResolution",       UINT8,      1);

The macro itself would leverage static_assert to validate that the declaration and the text name match.

#define CONSTRUCT_TYPED_SFIELD(sfName, txtName, stiSuffix, fieldValue, ...) \
SF_ ## stiSuffix const sfName ( \
access, STI_ ## stiSuffix, fieldValue, txtName, ##__VA_ARGS__); \
static_assert(std::string_view( #sfName) == "sf" txtName , \
    "Declaration of SField does not match its text name")

Do you think this might be be a preferable approach? If so, I'd be happy to code up the change as a [FOLD] for review.

[cjcobb23]

@cjcobb23 and @pwang200, it occurred to me that I can restructure the macros so the exact name of the SField is still findable by grepping. The revised macro relies of error checking, rather than correctness by construction, but it accomplishes the same end. The revised macro would result in declarations that look like this:

CONSTRUCT_TYPED_SFIELD(sfCloseResolution, "CloseResolution",       UINT8,      1);

The macro itself would leverage static_assert to validate that the declaration and the text name match.

#define CONSTRUCT_TYPED_SFIELD(sfName, txtName, stiSuffix, fieldValue, ...) \
SF_ ## stiSuffix const sfName ( \
access, STI_ ## stiSuffix, fieldValue, txtName, ##__VA_ARGS__); \
static_assert(std::string_view( #sfName) == "sf" txtName , \
    "Declaration of SField does not match its text name")

Do you think this might be be a preferable approach? If so, I'd be happy to code up the change as a [FOLD] for review.

@scottschurr I do prefer this. It accomplishes the same end, but now the exact name is findable by grep.

[scottschurr]

Hi folks, I changed up the macros and updated the construction parameters in the most recent [FOLD] commit. Let me know if you like this approach better. Thanks.

ps I'm sure it will eventually become blasé, but right now it is a rush to do string compares at compile time...

[pwang200]

Look great! Thanks for the change.

[cjcobb23]

Looks great

[scottschurr]

Thanks gents. I rebased and squashed. I'll mark the pull request as passed once I've seen it pass continuous integration.

[scottschurr]

Travis is passing all of the builds except for the macOS ones. There's a CI fix for macOS in a different pull request, so I'm inclined to say this pull request has done well enough on CI. I'm marking the pull request as passed.