[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: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!");
----------------
zahiraam wrote:
> rjmccall wrote:
> > `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.
> > `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.
> 
> Would that be an additional step in `initializeBuiltins` or part of step #4? I understants intuitively what the issue would be here if we didn't do that, but I can't find a test case that goes through that step. Would be clearer to me.
> 
> > - You could also just check for `NotBuiltin` and set `ObjCBuiltinID = 0` when you see it.
> 
> I don't think this can be done here. builtin::NotBuiltin is not known here in IdentierTable.h (get a lots of build errors). So, this needs to be done in `initializerBultins` method.
> Would that be an additional step in initializeBuiltins or part of step #4? I understants intuitively what the issue would be here if we didn't do that, but I can't find a test case that goes through that step. Would be clearer to me.

Yeah, it'd be step 4: instead of calling `Table.get(Name).setBuiltinID(Builtin::NotBuiltin);`, it would call `Table.get(Name).clearBuiltinID()`.

...although while you're editing that code, please change `Table.get(Name)` to `NameIt->second`.  We've already looked up the `IdentifierInfo` and don't need to do it again.

> I don't think this can be done here. builtin::NotBuiltin is not known here in IdentierTable.h (get a lots of build errors). So, this needs to be done in initializerBultins method.

Okay, that works for me.  Please at least assert that the argument to `setBuiltinID` is not this value.  It's acceptable to assume that the value is 0 — we're doing that already with the -1 / +1.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D146148/new/

https://reviews.llvm.org/D146148