zahiraam added inline comments.
================
Comment at: clang/include/clang/Basic/IdentifierTable.h:316
+ unsigned getInterestingIdentifierID() {
+ if (ObjCOrBuiltinID >= 1341 && ObjCOrBuiltinID < 1343)
+ return ObjCOrBuiltinID;
----------------
rjmccall wrote:
> This is closer to the right approach, thanks. I think the best way to do
> this is to put the ObjC keywords first, then the interesting identifiers,
> then the builtins. That means the builtin ID code above should check for /
> add / subtract `tok::NUM_OBJC_KEYWORDS + tok::NUM_INTERESTING_IDENTIFIERS`;
> please add a `FirstBuiltinID` constant for that in this class so that we can
> more easily rework things in the future (e.g. if we decide to stuff more
> things into this field). And then the code for InterestingIdentifierID
> should add/subtract `tok::NUM_OBJC_KEYWORDS`; again, please add a
> `FirstInterestingIdentifier` constant for that.
>
> Please make these two functions use `tok::InterestingIdentifierKind`. For
> `getInterestingIdentifierID()`, you'll have to decide how you want to
> represent that something isn't an interesting identifier. `ObjCKeywordKind`
> has a special enumerator (`not_keyword`) at value 0, so you could do the same
> thing here, but I think it might be better to use `llvm::Optional`.
> This is closer to the right approach, thanks. I think the best way to do
> this is to put the ObjC keywords first, then the interesting identifiers,
> then the builtins. That means the builtin ID code above should check for /
> add / subtract `tok::NUM_OBJC_KEYWORDS + tok::NUM_INTERESTING_IDENTIFIERS`;
> please add a `FirstBuiltinID` constant for that in this class so that we can
> more easily rework things in the future (e.g. if we decide to stuff more
> things into this field). And then the code for InterestingIdentifierID
> should add/subtract `tok::NUM_OBJC_KEYWORDS`; again, please add a
> `FirstInterestingIdentifier` constant for that.
>
I have made the changes in this new version of the patch.
> Please make these two functions use `tok::InterestingIdentifierKind`. For
> `getInterestingIdentifierID()`, you'll have to decide how you want to
> represent that something isn't an interesting identifier. `ObjCKeywordKind`
> has a special enumerator (`not_keyword`) at value 0, so you could do the same
> thing here, but I think it might be better to use `llvm::Optional`.
However, I am stumped for this. If we want to use
`tok::InterestingIdentifierKind`, then the addition of the interesting
Identifiers need to be made within IdentifierTable::Addkeywords (by adding a
new macro). If we do it the way it's done in this patch in
Builtin::Context::initializeBuiltins then `tok::InterestingIdentifierKind`
can't be used. Unless I am missing something?
Also, I left the `tok::not_interesting` only because not really sure how to use
the llvm::Optional.
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
