rjmccall added inline comments.
================
Comment at: clang/include/clang/Basic/IdentifierTable.h:85
+/// (including not_interesting).
+/// - The rest of the values represent builtin IDs (including not_builtin).
+static constexpr int FirstObjCKeywordID = 1;
----------------
The code below does not represent NotBuiltin (that's what adding and
subtracting 1 does).
================
Comment at: clang/include/clang/Basic/IdentifierTable.h:89
+static constexpr int FirstInterestingIdentifierID = LastObjCKeywordID + 1;
+static constexpr int LastInterestingIdentifierID = LastObjCKeywordID +
tok::NUM_INTERESTING_IDENTIFIERS;
+static constexpr int FirstBuiltinID = LastInterestingIdentifierID + 1;
----------------
I see that I had a bug in my suggestion: I had meant to write
`LastInterestingIdentifierID = FirstInterestingIdentifierID +
tok::NUM_INTERESTING_IDENTIFIERS - 2;` but I left it in terms of
`LastObjCKeywordID` instead, making the ranges off by 1. Your math fixes that;
sorry about that. I do think it would be clearer if each of these chained off
the last one, the way I meant to have it, though. So with your ranges (which
leave space to explicitly represent `not_interesting`), that would look like
`LastInterestingIdentifierID = FirstInterestingIdentifierID +
tok::NUM_INTERESTING_IDENTIFIERS - 1;`.
I'm not going to push you to not represent `not_interesting`, since you seem to
have deliberately changed things back that way, and I don't think it matters
that much. Although maybe you did that just because it didn't work in the code
I gave you? It would be more consistent with the other enums to not explicitly
represent `not_interesting`.
================
Comment at: clang/include/clang/Basic/IdentifierTable.h:325
void setBuiltinID(unsigned ID) {
- ObjCOrBuiltinID = ID + tok::NUM_OBJC_KEYWORDS;
- assert(ObjCOrBuiltinID - unsigned(tok::NUM_OBJC_KEYWORDS) == ID
- && "ID too large for field!");
+ ObjCOrBuiltinID = FirstBuiltinID + (ID - 1);
+ assert(getBuiltinID() == ID && "ID too large for field!");
----------------
`initializeBuiltins` does call `setBuiltinID(NotBuiltin)` in order to reset any
identifiers for which we have a `-fno-builtin=X` argument, so you need to
handle that somehow, because otherwise this is going to underflow and set up
the identifier with the last `InterestingIdentifierKind`. There are two
options:
- You could just make `initializeBuiltins` call some sort of `clearBuiltinID`
method that resets `ObjCOrBuiltinID` to 0. That has the advantage of making
the "lifecycle" of this field much more straightforward. For example, we
probably ought to be asserting in these methods that we're not overwriting
existing data; `clearBuiltinID` would know that it was okay to clear data, but
only from a builtin.
- You could also just check for `NotBuiltin` and set `ObjCBuiltinID = 0` when
you see it.
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D146148/new/
https://reviews.llvm.org/D146148
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
