[PATCH] D146148: Float_t and double_t types shouldn't be modified by #pragma clang fp eval_method

[John McCall via Phabricator via cfe-commits]

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