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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
