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

[Zahira Ammarguellat via Phabricator via cfe-commits]

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