[PATCH] D131683: Diagnosing the Future Keywords

[Erich Keane via Phabricator via cfe-commits]

erichkeane added inline comments.


================
Comment at: clang/include/clang/Basic/TokenKinds.def:387
 // C++11 keywords
-CXX11_KEYWORD(alignas               , 0)
+CXX11_KEYWORD(alignas               , KEYC23)
 // alignof and _Alignof return the required ABI alignment
----------------
Codesbyusman wrote:
> erichkeane wrote:
> > Hmm... this looks like it is going to be troublesome for the 'future' feature.  Can you make sure you have tests for all of these?  
> > Hmm... this looks like it is going to be troublesome for the 'future' feature.  Can you make sure you have tests for all of these?  
> 
> Yes working for the test cases
I meant, for tests you haven't written yet.  You need to write tests to validate that the future diagnostics are emitted properly.


================
Comment at: clang/lib/Lex/Preprocessor.cpp:799
+#define C23_KEYWORD(NAME, FLAGS) .Case(#NAME, diag::warn_c23_keyword)
+#define CXX11_KEYWORD(NAME, FLAGS) .Case(#NAME, diag::warn_c23_keyword)
+#include "clang/Basic/TokenKinds.def"
----------------
Codesbyusman wrote:
> erichkeane wrote:
> > This isn't right at all.  We should be looking through the list of flags instead of trying to assume that cxx11 keywords here are all 'future' C keywords.  First, this isn't true.  Second, even if it was, it is really fragile.
> > This isn't right at all.  We should be looking through the list of flags instead of trying to assume that cxx11 keywords here are all 'future' C keywords.  First, this isn't true.  Second, even if it was, it is really fragile
> 
> Yes I was looking to it, But not getting how to deal this.
> Will need to make the different Case to access some of the keywords that are define in CXX11_KEYWORD.
> Any suggestions?
I suspect this pattern of using the define over the  C99_KEYWORD/etc is not going to be sufficient.  You'll likely have to just define KEYWORD, use it to 'extract' the entire list of flags, then check to see which language flag is 'set' vs the current LangOpts.  You might be able to replace the C++ part of this that way to unify the implementations.

So something like:

    unsigned Flags = llvm::StringSwitch<unsigned>(II.getName())
    #define KEYWORD(NAME, FLAGS) .Case(#NAME, FLAGS)
    #include "clang/Basic/TokenKinds.def
    ;
    // Then, do a bunch of checks of the current 'lang' version vs the 'set' flag of the keyword.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D131683