[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/lib/Sema/SemaDecl.cpp:6784
+      if (II->getInterestingIdentifierID() != 0)
+        NewTD->addAttr(AvailableOnlyInDefaultEvalMethodAttr::Create(Context));
     }
----------------
zahiraam wrote:
> rjmccall wrote:
> > Please switch over the interesting identifiers here; we don't want to assume this feature is only used for these two names.
> > 
> > In fact, should we go ahead and immediately apply it to the four identifiers above this?  That would be nice, because then we could actually do this in two patches: one patch that does the refactor to track interesting identifiers but doesn't cause any functionality changes and a second, very small patch that just introduces the new special treatment for `float_t` and `double_t`.
> > Please switch over the interesting identifiers here; we don't want to assume this feature is only used for these two names.
> > 
> > In fact, should we go ahead and immediately apply it to the four identifiers above this?  That would be nice, because then we could actually do this in two patches: one patch that does the refactor to track interesting identifiers but doesn't cause any functionality changes and a second, very small patch that just introduces the new special treatment for `float_t` and `double_t`.
> 
> Are you saying that "FILE", "jmp_buf"," sigjmp_buf" and "ucontext_t" are also interesting identifiers? If yes, they should be added to the list of interesting identifiers in TokenKinds.def?
Right.  The basic idea of interesting identifiers is to replace these sorts of identifier comparisons in performance-critical code.  So your first patch would *only* add those four identifiers as interesting identifiers, handling them here by registering the `typedef` with the ASTContext like the code is already doing.  Then you'd make a follow-up patch that adds `float_t` and `double_t` and handles them here by implicitly adding your new attribute.


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

https://reviews.llvm.org/D146148