Keep the IdentifierInfo in the Token for alternative operator keyword

[ogoffart]

Summary

The goal of this commit is to fix clang-format so it does not merge tokens when
using the alternative spelling keywords. (eg: "not foo" should not become "notfoo")

The problem is that Preprocessor::HandleIdentifier used to drop the identifier info
from the token for these keyword. This means the first condition of
TokenAnnotator::spaceRequiredBefore is not met. We could add explicit check for
the spelling in that condition, but I think it is better to keep the IdentifierInfo
and handle the operator keyword explicitly when needed. That actually leads to simpler
code, and probably slightly more efficient as well.

Another side effect of this change is that __identifier(and) will now work as
one would expect, removing a FIXME from the MicrosoftExtensions.cpp test

[erichkeane]

My interaction with this is as a submitter for Melanie, so hopefully she can validate that this doesn't break anything subtle. To me it DOES seem like a good approach, however Richard likely has a better idea about it, so I want to give him a bit of time before accepting.

[rsmith]

It looks like there are fewer special cases with this direction then our present one, though I worry that they'll be less obvious. On the whole, this seems like a improvement.

lib/Lex/PPExpressions.cpp

242 ↗

(On Diff #105744)

I'm concerned that this will do the wrong thing for a keyword operator that is not permitted in a pp expression, like and_eq. It seems safer to revert this change and instead add a isCPlusPlusOperatorKeyword check to the "if" condition above.

[ogoffart]

lib/Lex/PPExpressions.cpp

242 ↗

(On Diff #105744)

Well spotted. I kept this code in the default: so true and false are handled separatly, but added a condition for isCPlusPlusOperatorKeyword, and an additional test.

[ogoffart]

Added check for "#if and_eq"

cfe/trunk/include/clang/Basic/IdentifierTable.h

public:
 
   /// \brief Return true if this token has been poisoned.
   bool isPoisoned() const { return IsPoisoned; }
 
   /// isCPlusPlusOperatorKeyword/setIsCPlusPlusOperatorKeyword controls whether
   /// this identifier is a C++ alternate representation of an operator.
   void setIsCPlusPlusOperatorKeyword(bool Val = true) {
     IsCPPOperatorKeyword = Val;
-    if (Val)
-      NeedsHandleIdentifier = true;
-    else
-      RecomputeNeedsHandleIdentifier();
   }
   bool isCPlusPlusOperatorKeyword() const { return IsCPPOperatorKeyword; }
 
   /// \brief Return true if this token is a keyword in the specified language.
   bool isKeyword(const LangOptions &LangOpts) const;
 
   /// \brief Return true if this token is a C++ keyword in the specified
   /// language.

 private:
   /// The Preprocessor::HandleIdentifier does several special (but rare)
   /// things to identifiers of various sorts.  For example, it changes the
   /// \c for keyword token from tok::identifier to tok::for.
   ///
   /// This method is very tied to the definition of HandleIdentifier.  Any
   /// change to it should be reflected here.
   void RecomputeNeedsHandleIdentifier() {
-    NeedsHandleIdentifier =
-      (isPoisoned() | hasMacroDefinition() | isCPlusPlusOperatorKeyword() |
-       isExtensionToken() | isFutureCompatKeyword() || isOutOfDate() ||
-       isModulesImport());
+    NeedsHandleIdentifier = isPoisoned() || hasMacroDefinition() ||
+                            isExtensionToken() || isFutureCompatKeyword() ||
+                            isOutOfDate() || isModulesImport();
   }
 };
 
 /// \brief An RAII object for [un]poisoning an identifier within a scope.
 ///
 /// \p II is allowed to be null, in which case objects of this type have
 /// no effect.
 class PoisonIdentifierRAIIObject {

cfe/trunk/lib/Lex/PPDirectives.cpp

 
 bool Preprocessor::CheckMacroName(Token &MacroNameTok, MacroUse isDefineUndef,
                                   bool *ShadowFlag) {
   // Missing macro name?
   if (MacroNameTok.is(tok::eod))
     return Diag(MacroNameTok, diag::err_pp_missing_macro_name);
 
   IdentifierInfo *II = MacroNameTok.getIdentifierInfo();
-  if (!II) {
-    bool Invalid = false;
-    std::string Spelling = getSpelling(MacroNameTok, &Invalid);
-    if (Invalid)
-      return Diag(MacroNameTok, diag::err_pp_macro_not_identifier);
-    II = getIdentifierInfo(Spelling);
-
-    if (!II->isCPlusPlusOperatorKeyword())
-      return Diag(MacroNameTok, diag::err_pp_macro_not_identifier);
+  if (!II)
 
+  if (II->isCPlusPlusOperatorKeyword()) {
     // C++ 2.5p2: Alternative tokens behave the same as its primary token
     // except for their spellings.
     Diag(MacroNameTok, getLangOpts().MicrosoftExt
                            ? diag::ext_pp_operator_used_as_macro_name
                            : diag::err_pp_operator_used_as_macro_name)
         << II << MacroNameTok.getKind();
-
     // Allow #defining |and| and friends for Microsoft compatibility or
     // recovery when legacy C headers are included in C++.
-    MacroNameTok.setIdentifierInfo(II);
   }
 
   if ((isDefineUndef != MU_Other) && II->getPPKeywordID() == tok::pp_defined) {
     // Error if defining "defined": C99 6.10.8/4, C++ [cpp.predefined]p4.
     return Diag(MacroNameTok, diag::err_defined_macro_name);
   }
 
   if (isDefineUndef == MU_Undef) {

cfe/trunk/lib/Lex/PPExpressions.cpp

static bool EvaluateValue(PPValue &Result, Token &PeekTok, DefinedTracker &DT,
   Result.setIdentifier(nullptr);
 
   if (PeekTok.is(tok::code_completion)) {
     if (PP.getCodeCompletionHandler())
       PP.getCodeCompletionHandler()->CodeCompletePreprocessorExpression();
     PP.setCodeCompletionReached();
     PP.LexNonComment(PeekTok);
   }
-      
+  switch (PeekTok.getKind()) {
+  default:
     // If this token's spelling is a pp-identifier, check to see if it is
     // 'defined' or if it is a macro.  Note that we check here because many
     // keywords are pp-identifiers, so we can't check the kind.
     if (IdentifierInfo *II = PeekTok.getIdentifierInfo()) {
       // Handle "defined X" and "defined(X)".
       if (II->isStr("defined"))
         return EvaluateDefined(Result, PeekTok, DT, ValueLive, PP);
 
+      if (!II->isCPlusPlusOperatorKeyword()) {
         // If this identifier isn't 'defined' or one of the special
         // preprocessor keywords and it wasn't macro expanded, it turns
-    // into a simple 0, unless it is the C++ keyword "true", in which case it
-    // turns into "1".
-    if (ValueLive &&
-        II->getTokenID() != tok::kw_true &&
-        II->getTokenID() != tok::kw_false)
+        // into a simple 0
+        if (ValueLive)
           PP.Diag(PeekTok, diag::warn_pp_undef_identifier) << II;
-    Result.Val = II->getTokenID() == tok::kw_true;
-    Result.Val.setIsUnsigned(false);  // "0" is signed intmax_t 0.
+        Result.Val = 0;
+        Result.Val.setIsUnsigned(false); // "0" is signed intmax_t 0.
         Result.setIdentifier(II);
         Result.setRange(PeekTok.getLocation());
-    DT.IncludedUndefinedIds = (II->getTokenID() != tok::kw_true &&
-                               II->getTokenID() != tok::kw_false);
+        DT.IncludedUndefinedIds = true;
         PP.LexNonComment(PeekTok);
         return false;
       }
-
-  switch (PeekTok.getKind()) {
-  default:  // Non-value token.
+    }
     PP.Diag(PeekTok, diag::err_pp_expr_bad_token_start_expr);
     return true;
   case tok::eod:
   case tok::r_paren:
     // If there is no expression, report and exit.
     PP.Diag(PeekTok, diag::err_pp_expected_value_in_expr);
     return true;
   case tok::numeric_constant: {
case tok::exclaim: {
     Result.setIdentifier(nullptr);
 
     if (DT.State == DefinedTracker::DefinedMacro)
       DT.State = DefinedTracker::NotDefinedMacro;
     else if (DT.State == DefinedTracker::NotDefinedMacro)
       DT.State = DefinedTracker::DefinedMacro;
     return false;
   }
+  case tok::kw_true:
+  case tok::kw_false:
+    Result.Val = PeekTok.getKind() == tok::kw_true;
+    Result.Val.setIsUnsigned(false); // "0" is signed intmax_t 0.
+    Result.setIdentifier(PeekTok.getIdentifierInfo());
+    Result.setRange(PeekTok.getLocation());
+    PP.LexNonComment(PeekTok);
+    return false;
 
   // FIXME: Handle #assert
   }
 }
 
 /// getPrecedence - Return the precedence of the specified binary operator
 /// token.  This returns:
 ///   ~0 - Invalid token.

cfe/trunk/lib/Lex/Preprocessor.cpp

bool Preprocessor::HandleIdentifier(Token &Identifier) {
   //   "#define constexpr constexpr", "int constexpr;"
   if (II.isFutureCompatKeyword() && !DisableMacroExpansion) {
     Diag(Identifier, getFutureCompatDiagKind(II, getLangOpts()))
         << II.getName();
     // Don't diagnose this keyword again in this translation unit.
     II.setIsFutureCompatKeyword(false);
   }
 
-  // C++ 2.11p2: If this is an alternative representation of a C++ operator,
-  // then we act as if it is the actual operator and not the textual
-  // representation of it.
-  if (II.isCPlusPlusOperatorKeyword() &&
-      !(getLangOpts().MSVCCompat &&
-        getSourceManager().isInSystemHeader(Identifier.getLocation())))
-    Identifier.setIdentifierInfo(nullptr);
-
   // If this is an extension token, diagnose its use.
   // We avoid diagnosing tokens that originate from macro definitions.
   // FIXME: This warning is disabled in cases where it shouldn't be,
   // like "#define TY typeof", "TY(1) x".
   if (II.isExtensionToken() && !DisableMacroExpansion)
     Diag(Identifier, diag::ext_token_used);
   
   // If this is the 'import' contextual keyword following an '@', note

cfe/trunk/test/Parser/MicrosoftExtensions.cpp

 struct __identifier(class) { __identifier(class) *__identifier(for); };
 __identifier(class) __identifier(struct) = { &__identifier(struct) };
 
 int __identifier for; // expected-error {{missing '(' after '__identifier'}}
 int __identifier(else} = __identifier(for); // expected-error {{missing ')' after identifier}} expected-note {{to match this '('}}
 #define identifier_weird(x) __identifier(x
 int k = identifier_weird(if)); // expected-error {{use of undeclared identifier 'if'}}
 
-// This is a bit weird, but the alternative tokens aren't keywords, and this
-// behavior matches MSVC. FIXME: Consider supporting this anyway.
-extern int __identifier(and) r; // expected-error {{cannot convert '&&' token to an identifier}}
+extern int __identifier(and);
 
 void f() {
   __identifier(() // expected-error {{cannot convert '(' token to an identifier}}
   __identifier(void) // expected-error {{use of undeclared identifier 'void'}}
   __identifier()) // expected-error {{cannot convert ')' token to an identifier}}
   // FIXME: We should pick a friendlier display name for this token kind.
   __identifier(1) // expected-error {{cannot convert <numeric_constant> token to an identifier}}
   __identifier(+) // expected-error {{cannot convert '+' token to an identifier}}

cfe/trunk/test/Preprocessor/cxx_oper_keyword.cpp

 // For error recovery we continue as though the identifier was a macro name regardless of -fno-operator-names.
 #ifdef OPERATOR_NAMES
 //expected-error@+3 {{C++ operator 'and' (aka '&&') used as a macro name}}
 #endif
 //expected-warning@+2 {{and is defined}}
 #ifdef and
 #warning and is defined
 #endif
+
+#ifdef OPERATOR_NAMES
+//expected-error@+2 {{invalid token at start of a preprocessor expression}}
+#endif
+#if or
+#endif
+
+#ifdef OPERATOR_NAMES
+//expected-error@+2 {{invalid token at start of a preprocessor expression}}
+#endif
+#if and_eq
+#endif

cfe/trunk/unittests/Format/FormatTest.cpp

TEST_F(FormatTest, RecognizesBinaryOperatorKeywords) {
   verifyFormat("x = (a) bitand (b);");
   verifyFormat("x = (a) bitor (b);");
   verifyFormat("x = (a) not_eq (b);");
   verifyFormat("x = (a) and_eq (b);");
   verifyFormat("x = (a) or_eq (b);");
   verifyFormat("x = (a) xor (b);");
 }
 
+TEST_F(FormatTest, RecognizesUnaryOperatorKeywords) {
+  verifyFormat("x = compl(a);");
+  verifyFormat("x = not(a);");
+  verifyFormat("x = bitand(a);");
+  // Unary operator must not be merged with the next identifier
+  verifyFormat("x = compl a;");
+  verifyFormat("x = not a;");
+  verifyFormat("x = bitand a;");
+}
+
 //===----------------------------------------------------------------------===//
 // Tests for control statements.
 //===----------------------------------------------------------------------===//
 
 TEST_F(FormatTest, FormatIfWithoutCompoundStatement) {
   verifyFormat("if (true)\n  f();\ng();");
   verifyFormat("if (a)\n  if (b)\n    if (c)\n      g();\nh();");
   verifyFormat("if (a)\n  if (b) {\n    f();\n  }\ng();");