RecordType::DELETE to RecordType::DELETE_FLAG
#34454
On Windows, the winnt.h header defines DELETE as a macro for a “Standard Access Right” bitmask (0x00010000L).
This introduces a fragile dependency on header inclusion order: if Windows headers happen to be included before the RecordType enum definition, the preprocessor expands DELETE into a numeric literal, causing syntax errors.
Rename the enumerator to DELETE_FLAG to remove this fragility and avoid the collision entirely.
Split from #34448.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
See the guideline for information on the review process.
If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.
Seems fine, but I think it is annoying that we keep running into the same bug over and over again (IIRC it happened in the past at least once for Bitcoin Core). Surely this is going to happen again in the future.
I think it may be good to enforce https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#renum-caps (Enum.5: Don’t use ALL_CAPS for enumerators)
I have not tried this at all, but i think with something like this, we can use clang-tidy to reject ALL_CAPS:
0 - key: readability-identifier-naming.EnumConstantIgnoredRegexp
1 value: '.*[a-z].*' # ignore if there is any lower case letter
2 - key: readability-identifier-naming.EnumConstantCase
3 value: CamelCase # otherwise, suggest this
4
5 - key: readability-identifier-naming.ScopedEnumConstantIgnoredRegexp
6 value: '.*[a-z].*' # ignore if there is any lower case letter
7 - key: readability-identifier-naming.ScopedEnumConstantCase
8 value: CamelCase # otherwise, suggest this
9...
Seems fine, but I think it is annoying that we keep running into the same bug over and over again (IIRC it happened in the past at least once for Bitcoin Core). Surely this is going to happen again in the future.
I think it may be good to enforce https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#renum-caps (Enum.5: Don’t use ALL_CAPS for enumerators)
I have not tried this at all, but i think with something like this, we can use clang-tidy to reject ALL_CAPS:
0 - key: readability-identifier-naming.EnumConstantIgnoredRegexp 1 value: '.*[a-z].*' # ignore if there is any lower case letter 2 - key: readability-identifier-naming.EnumConstantCase 3 value: CamelCase # otherwise, suggest this 4 5 - key: readability-identifier-naming.ScopedEnumConstantIgnoredRegexp 6 value: '.*[a-z].*' # ignore if there is any lower case letter 7 - key: readability-identifier-naming.ScopedEnumConstantCase 8 value: CamelCase # otherwise, suggest this 9...
I considered this while working on the PR, but enforcing that rule globally would be extremely invasive. For example, it would require renaming in enum opcodetype.
lgtm ACK e10d135a9a64e1b35faea70f84abdd3ef5dd0bd1
Further improvements can be considered later.
On Windows, the `winnt.h` header defines `DELETE` as a macro for a
"Standard Access Right" bitmask (0x00010000L).
This introduces a fragile dependency on header inclusion order: if
Windows headers happen to be included before this enum definition,
the preprocessor expands `DELETE` into a numeric literal, causing
syntax errors.
Rename the enumerator to `DELETE_FLAG` to remove this fragility and
avoid the collision entirely.
207@@ -208,8 +208,8 @@ class RecordHeader
208 {
209 public:
210 uint16_t len; // Key/data item length
211- RecordType type; // Page type (BDB has this include a DELETE FLAG that we track separately)
212- bool deleted; // Whether the DELETE flag was set on type
213+ RecordType type; // Page type (BDB has this include a DELETE_FLAG that we track separately)
0 RecordType type; // Page type (BDB has this; includes a DELETE_FLAG that we track separately)
llm typo nit
