Consolidate and improve the handling of built-in feature-like macros

[AndyG]

Summary

The parsing logic has been separated out from the macro implementation logic, leading to a number of improvements:

• Gracefully handle unexpected/invalid tokens, too few, too many and nested parameters
• Provide consistent behaviour between all built-in feature-like macros
• Simplify the implementation of macro logic
• Fix __is_identifier to correctly return '0' for non-identifiers

[AndyG]

Test comment -- just to see whether email notifications are sending properly (it seems, for example, that cfe-commits wasn't notified of this patch...)

[rsmith]

lib/Lex/PPMacroExpansion.cpp

1451–1467

Why should this be treated as valid? That's not part of the specification for these features.

[AndyG]

To be honest, the simple answer is because it was just as easy to do with nesting as without (the code would still need to track the appearance of left and right parentheses in order to correctly parse to the closing right-parenthesis of the macro invocation in any case).

And since the work has to be done anyway, it seemed reasonable to allow it. My other thought was that when people write wrapper macros, they often (indeed idiomatically) wrap their parameters in an extra set of parentheses before passing down to the next level. This would therefore handle this use-case.

While it may not technically be part of the specification, neither does it break it, and in the 99% most common cases the extra functionality will not come into play.

The motivation of the patch was to ensure a more robust parsing of these feature-like macros -- that and fixing __is_identifier which was fundamentally broken and where I originally started.

If you really disagree with this extension of the rules noted in your comment, I can work a logic that errors on the presence of embedded parentheses, but IMHO I think it preferable to keep it as is :o)

[AndyG]

Bump :o)

[AndyG]

Second bump :o)

[AndyG]

Ok, I've removed support for nested parentheses. Can this go through now?

Thanks.

[rsmith]

lib/Lex/PPMacroExpansion.cpp

1430–1432

Use llvm::function_ref here instead, and don't pass a Preprocessor into the function (instead, capture it in a lambda).

1456–1457

If we get an annotation token here, we should reject it, not silently ignore it. Also, we shouldn't see comment tokens here (we shouldn't be doing macro expansion with comments enabled); you should call LexUnexpandedToken rather than LexUnexpandedNonComment.

1481–1484

The only way we can get here without already having a value or producing a diagnostic is if this is the first token inside the parens. So this will always say "unexpected '(' after '('".

I think it would be better to always break here after incrementing ParenDepth (even when !Result.hasValue()), and let Op produce the relevant diagnostic for this case.

1519

expected -> missing

[AndyG]

Implemented comments.

lib/Lex/PPMacroExpansion.cpp

1456–1457

Ok, annotation tokens are not ignored, but drop down into Op and can be handled there.

1481–1484

I've handled this via a specific diagnostic message: "nested parentheses not permitted in %0" where %0 is the macro name. I would prefer this to bringing the error checking into Op since this would complicate the implementation of Op needlessly IMHO. Op should be as slimline as possible.

[rsmith]

lib/Lex/PPMacroExpansion.cpp

1481–1484

I think this can be done without any changes to any Op implementation: each of them already diagnoses a left paren just as it would diagnose any other unexpected token. But the custom diagnostic also seems fine.

include/clang/Basic/DiagnosticLexKinds.td

 def pp_err_else_without_if : Error<"#else without #if">;
 def pp_err_elif_without_if : Error<"#elif without #if">;
 def err_pp_endif_without_if : Error<"#endif without #if">;
 def err_pp_expected_value_in_expr : Error<"expected value in expression">;
 def err_pp_expected_rparen : Error<"expected ')' in preprocessor expression">;
 def err_pp_expected_eol : Error<
   "expected end of line in preprocessor expression">;
 def err_pp_expected_after : Error<"missing %1 after %0">;
+def err_pp_unexpected_after : Error<"%1 not expected after %0">;
 def err_pp_colon_without_question : Error<"':' without preceding '?'">;
 def err_pp_division_by_zero : Error<
   "division by zero in preprocessor expression">;
 def err_pp_remainder_by_zero : Error<
   "remainder by zero in preprocessor expression">;
 def err_pp_expr_bad_token_binop : Error<
   "token is not a valid binary operator in a preprocessor subexpression">;
 def err_pp_expr_bad_token_start_expr : Error<

lib/Lex/PPMacroExpansion.cpp

static const char * const Months[] = {
     PP.CreateString(TmpStream.str(), TmpTok);
     TIMELoc = TmpTok.getLocation();
   }
 }
 
 
 /// HasFeature - Return true if we recognize and implement the feature
 /// specified by the identifier as a standard language feature.
-static bool HasFeature(const Preprocessor &PP, const IdentifierInfo *II) {
+static bool HasFeature(const Preprocessor &PP, StringRef Feature) {
   const LangOptions &LangOpts = PP.getLangOpts();
-  StringRef Feature = II->getName();
 
   // Normalize the feature name, __foo__ becomes foo.
   if (Feature.startswith("__") && Feature.endswith("__") && Feature.size() >= 4)
     Feature = Feature.substr(2, Feature.size() - 4);
 
   return llvm::StringSwitch<bool>(Feature)
       .Case("address_sanitizer",
             LangOpts.Sanitize.hasOneOf(SanitizerKind::Address |
return llvm::StringSwitch<bool>(Feature)
       .Case("tls", PP.getTargetInfo().isTLSSupported())
       .Case("underlying_type", LangOpts.CPlusPlus)
       .Default(false);
 }
 
 /// HasExtension - Return true if we recognize and implement the feature
 /// specified by the identifier, either as an extension or a standard language
 /// feature.
-static bool HasExtension(const Preprocessor &PP, const IdentifierInfo *II) {
-  if (HasFeature(PP, II))
+static bool HasExtension(const Preprocessor &PP, StringRef Extension) {
+  if (HasFeature(PP, Extension))
     return true;
 
   // If the use of an extension results in an error diagnostic, extensions are
   // effectively unavailable, so just return false here.
   if (PP.getDiagnostics().getExtensionHandlingBehavior() >=
       diag::Severity::Error)
     return false;
 
   const LangOptions &LangOpts = PP.getLangOpts();
-  StringRef Extension = II->getName();
 
   // Normalize the extension name, __foo__ becomes foo.
   if (Extension.startswith("__") && Extension.endswith("__") &&
       Extension.size() >= 4)
     Extension = Extension.substr(2, Extension.size() - 4);
 
   // Because we inherit the feature list from HasFeature, this string switch
   // must be less restrictive than HasFeature's.
static bool EvaluateHasIncludeNext(Token &Tok,
   } else {
     // Start looking up in the next directory.
     ++Lookup;
   }
 
   return EvaluateHasIncludeCommon(Tok, II, PP, Lookup, LookupFromFile);
 }
 
-/// \brief Process __building_module(identifier) expression.
-/// \returns true if we are building the named module, false otherwise.
-static bool EvaluateBuildingModule(Token &Tok,
-                                   IdentifierInfo *II, Preprocessor &PP) {
-  // Get '('.
-  PP.LexNonComment(Tok);
-
-  // Ensure we have a '('.
+/// \brief Process single-argument builtin feature-like macros that return
+/// integer values.
+static void EvaluateFeatureLikeBuiltinMacro(llvm::raw_svector_ostream& OS,
+                                            Token &Tok, IdentifierInfo *II,
+                                            Preprocessor &PP,
+                                            int(*Op)(Token &Tok,
+                                                     Preprocessor &PP,
+                                                     bool &HasLexedNextTok)) {
+  // Parse the initial '('.
+  PP.LexUnexpandedNonComment(Tok);
   if (Tok.isNot(tok::l_paren)) {
     PP.Diag(Tok.getLocation(), diag::err_pp_expected_after) << II
                                                             << tok::l_paren;
-    return false;
+
+    // Provide a dummy '0' value on output stream to elide further errors.
+    if (!Tok.isOneOf(tok::eof, tok::eod)) {
+      OS << 0;
+      Tok.setKind(tok::numeric_constant);
+    }
+    return;
   }
 
-  // Save '(' location for possible missing ')' message.
+  unsigned ParenDepth = 1;
   SourceLocation LParenLoc = Tok.getLocation();
+  llvm::Optional<int> Result;
 
-  // Get the module name.
-  PP.LexNonComment(Tok);
+  bool SuppressDiagnostic = false;
+  while (true) {
+    // Keep last token for diagnostics.
+    Token LastTok = Tok;
 
-  // Ensure that we have an identifier.
-  if (Tok.isNot(tok::identifier)) {
-    PP.Diag(Tok.getLocation(), diag::err_expected_id_building_module);
-    return false;
+    // Parse next non-comment, non-annotation token.
+    do PP.LexUnexpandedNonComment(Tok); while (Tok.isAnnotation());
+
+already_lexed:
+    switch (Tok.getKind()) {
+      case tok::eof:
+      case tok::eod:
+        // Don't provide even a dummy value if the eod or eof marker is
+        // reached.  Simply provide a diagnostic.
+        PP.Diag(Tok.getLocation(), diag::err_unterm_macro_invoc);
+        return;
+
+      case tok::comma:
+        if (!SuppressDiagnostic) {
+          PP.Diag(Tok.getLocation(), diag::err_too_many_args_in_macro_invoc);
+          SuppressDiagnostic = true;
         }
+        continue;
 
-  bool Result =
-      PP.getLangOpts().CompilingModule &&
-      Tok.getIdentifierInfo()->getName() == PP.getLangOpts().CurrentModule;
+      case tok::l_paren:
+        ++ParenDepth;
+        if (Result.hasValue())
+          break;
+        if (!SuppressDiagnostic) {
+          auto Diag = PP.Diag(Tok.getLocation(), diag::err_pp_unexpected_after);
+          if (IdentifierInfo *LastII = LastTok.getIdentifierInfo())
+            Diag << LastII;
+          else
+            Diag << LastTok.getKind();
+          Diag << tok::l_paren << LastTok.getLocation();
+          SuppressDiagnostic = true;
+        }
+        continue;
 
-  // Get ')'.
-  PP.LexNonComment(Tok);
+      case tok::r_paren:
+        if (--ParenDepth > 0)
+          continue;
 
-  // Ensure we have a trailing ).
-  if (Tok.isNot(tok::r_paren)) {
-    PP.Diag(Tok.getLocation(), diag::err_pp_expected_after) << II
-                                                            << tok::r_paren;
+        // The last ')' has been reached; return the value if one found or
+        // a diagnostic and a dummy value.
+        if (Result.hasValue())
+          OS << Result.getValue();
+        else {
+          OS << 0;
+          if (!SuppressDiagnostic)
+            PP.Diag(Tok.getLocation(), diag::err_too_few_args_in_macro_invoc);
+        }
+        Tok.setKind(tok::numeric_constant);
+        return;
+
+      default: {
+        // Parse the macro argument, if one not found so far.
+        if (Result.hasValue())
+          break;
+
+        bool HasLexedNextToken = false;
+        Result = Op(Tok, PP, HasLexedNextToken);
+        if (HasLexedNextToken)
+          goto already_lexed;
+        continue;
+      }
+    }
+
+    // Diagnose expected ')'.
+    if (!SuppressDiagnostic) {
+      if (auto Diag = PP.Diag(Tok.getLocation(), diag::err_pp_expected_after)) {
+        if (IdentifierInfo *LastII = LastTok.getIdentifierInfo())
+          Diag << LastII;
+        else
+          Diag << LastTok.getKind();
+        Diag << tok::r_paren << LastTok.getLocation();
+      }
       PP.Diag(LParenLoc, diag::note_matching) << tok::l_paren;
-    return false;
+      SuppressDiagnostic = true;
     }
+  }
+}
+
+/// \brief Helper function to return the IdentifierInfo structure of a Token
+/// or generate a diagnostic if none available.
+static IdentifierInfo *ExpectFeatureIdentifierInfo(Token &Tok,
+                                                   Preprocessor &PP,
+                                                   signed DiagID) {
+  IdentifierInfo *II;
+  if (!Tok.isAnnotation() && (II = Tok.getIdentifierInfo()))
+    return II;
 
-  return Result;
+  PP.Diag(Tok.getLocation(), DiagID);
+  return nullptr;
 }
 
 /// ExpandBuiltinMacro - If an identifier token is read that is to be expanded
 /// as a builtin macro, handle it and return the next token as 'Tok'.
 void Preprocessor::ExpandBuiltinMacro(Token &Tok) {
   // Figure out which token this is.
   IdentifierInfo *II = Tok.getIdentifierInfo();
   assert(II && "Can't be a macro without id info!");
if (II == Ident__LINE__) {
     }
     // Surround the string with " and strip the trailing newline.
     OS << '"' << StringRef(Result).drop_back() << '"';
     Tok.setKind(tok::string_literal);
   } else if (II == Ident__COUNTER__) {
     // __COUNTER__ expands to a simple numeric value.
     OS << CounterValue++;
     Tok.setKind(tok::numeric_constant);
-  } else if (II == Ident__has_feature   ||
-             II == Ident__has_extension ||
-             II == Ident__has_builtin   ||
-             II == Ident__is_identifier ||
-             II == Ident__has_attribute ||
-             II == Ident__has_declspec  ||
-             II == Ident__has_cpp_attribute) {
-    // The argument to these builtins should be a parenthesized identifier.
-    SourceLocation StartLoc = Tok.getLocation();
-
-    bool IsValid = false;
-    IdentifierInfo *FeatureII = nullptr;
-    IdentifierInfo *ScopeII = nullptr;
-
-    // Read the '('.
-    LexUnexpandedToken(Tok);
-    if (Tok.is(tok::l_paren)) {
-      // Read the identifier
-      LexUnexpandedToken(Tok);
-      if ((FeatureII = Tok.getIdentifierInfo())) {
-        // If we're checking __has_cpp_attribute, it is possible to receive a
-        // scope token. Read the "::", if it's available.
-        LexUnexpandedToken(Tok);
-        bool IsScopeValid = true;
-        if (II == Ident__has_cpp_attribute && Tok.is(tok::coloncolon)) {
-          LexUnexpandedToken(Tok);
-          // The first thing we read was not the feature, it was the scope.
-          ScopeII = FeatureII;
-          if ((FeatureII = Tok.getIdentifierInfo()))
-            LexUnexpandedToken(Tok);
-          else
-            IsScopeValid = false;          
-        }
-        // Read the closing paren.
-        if (IsScopeValid && Tok.is(tok::r_paren))
-          IsValid = true;
-      }
-      // Eat tokens until ')'.
-      while (Tok.isNot(tok::r_paren) && Tok.isNot(tok::eod) &&
-             Tok.isNot(tok::eof))
-        LexUnexpandedToken(Tok);
-    }
-
-    int Value = 0;
-    if (!IsValid)
-      Diag(StartLoc, diag::err_feature_check_malformed);
-    else if (II == Ident__is_identifier)
-      Value = FeatureII->getTokenID() == tok::identifier;
-    else if (II == Ident__has_builtin) {
-      // Check for a builtin is trivial.
-      if (FeatureII->getBuiltinID() != 0) {
-        Value = true;
-      } else {
-        StringRef Feature = FeatureII->getName();
-        Value = llvm::StringSwitch<bool>(Feature)
-                    .Case("__make_integer_seq", getLangOpts().CPlusPlus)
+  } else if (II == Ident__has_feature) {
+    EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
+      [](Token &Tok, Preprocessor &PP, bool &HasLexedNextToken) -> int {
+        IdentifierInfo *II = ExpectFeatureIdentifierInfo(Tok, PP,
+                                           diag::err_feature_check_malformed);
+        return II && HasFeature(PP, II->getName());
+      });
+  } else if (II == Ident__has_extension) {
+    EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
+      [](Token &Tok, Preprocessor &PP, bool &HasLexedNextToken) -> int {
+        IdentifierInfo *II = ExpectFeatureIdentifierInfo(Tok, PP,
+                                           diag::err_feature_check_malformed);
+        return II && HasExtension(PP, II->getName());
+      });
+  } else if (II == Ident__has_builtin) {
+    EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
+      [](Token &Tok, Preprocessor &PP, bool &HasLexedNextToken) -> int {
+        IdentifierInfo *II = ExpectFeatureIdentifierInfo(Tok, PP,
+                                           diag::err_feature_check_malformed);
+        if (!II)
+          return false;
+        else if (II->getBuiltinID() != 0)
+          return true;
+        else {
+          const LangOptions &LangOpts = PP.getLangOpts();
+          return llvm::StringSwitch<bool>(II->getName())
+                      .Case("__make_integer_seq", LangOpts.CPlusPlus)
                       .Default(false);
         }
-    } else if (II == Ident__has_attribute)
-      Value = hasAttribute(AttrSyntax::GNU, nullptr, FeatureII,
-                           getTargetInfo(), getLangOpts());
-    else if (II == Ident__has_cpp_attribute)
-      Value = hasAttribute(AttrSyntax::CXX, ScopeII, FeatureII,
-                           getTargetInfo(), getLangOpts());
-    else if (II == Ident__has_declspec)
-      Value = hasAttribute(AttrSyntax::Declspec, nullptr, FeatureII,
-                           getTargetInfo(), getLangOpts());
-    else if (II == Ident__has_extension)
-      Value = HasExtension(*this, FeatureII);
+      });
+  } else if (II == Ident__is_identifier) {
+    EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
+      [](Token &Tok, Preprocessor &PP, bool &HasLexedNextToken) -> int {
+        return Tok.is(tok::identifier);
+      });
+  } else if (II == Ident__has_attribute) {
+    EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
+      [](Token &Tok, Preprocessor &PP, bool &HasLexedNextToken) -> int {
+        IdentifierInfo *II = ExpectFeatureIdentifierInfo(Tok, PP,
+                                           diag::err_feature_check_malformed);
+        return II ? hasAttribute(AttrSyntax::GNU, nullptr, II,
+                                 PP.getTargetInfo(), PP.getLangOpts()) : 0;
+      });
+  } else if (II == Ident__has_declspec) {
+    EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
+      [](Token &Tok, Preprocessor &PP, bool &HasLexedNextToken) -> int {
+        IdentifierInfo *II = ExpectFeatureIdentifierInfo(Tok, PP,
+                                           diag::err_feature_check_malformed);
+        return II ? hasAttribute(AttrSyntax::Declspec, nullptr, II,
+                                 PP.getTargetInfo(), PP.getLangOpts()) : 0;
+      });
+  } else if (II == Ident__has_cpp_attribute) {
+    EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
+      [](Token &Tok, Preprocessor &PP, bool &HasLexedNextToken) -> int {
+        IdentifierInfo *ScopeII = nullptr;
+        IdentifierInfo *II = ExpectFeatureIdentifierInfo(Tok, PP,
+                                           diag::err_feature_check_malformed);
+        if (!II)
+          return false;
+
+        // It is possible to receive a scope token.  Read the "::", if it is
+        // available, and the subsequent identifier.
+        PP.LexUnexpandedToken(Tok);
+        if (Tok.isNot(tok::coloncolon))
+          HasLexedNextToken = true;
         else {
-      assert(II == Ident__has_feature && "Must be feature check");
-      Value = HasFeature(*this, FeatureII);
+          ScopeII = II;
+          PP.LexUnexpandedToken(Tok);
+          II = ExpectFeatureIdentifierInfo(Tok, PP,
+                                           diag::err_feature_check_malformed);
         }
 
-    if (!IsValid)
-      return;
-    OS << Value;
-    Tok.setKind(tok::numeric_constant);
+        return II ? hasAttribute(AttrSyntax::CXX, ScopeII, II,
+                                 PP.getTargetInfo(), PP.getLangOpts()) : 0;
+      });
   } else if (II == Ident__has_include ||
              II == Ident__has_include_next) {
     // The argument to these two builtins should be a parenthesized
     // file name string literal using angle brackets (<>) or
     // double-quotes ("").
     bool Value;
     if (II == Ident__has_include)
       Value = EvaluateHasInclude(Tok, II, *this);
     else
       Value = EvaluateHasIncludeNext(Tok, II, *this);
 
     if (Tok.isNot(tok::r_paren))
       return;
     OS << (int)Value;
     Tok.setKind(tok::numeric_constant);
   } else if (II == Ident__has_warning) {
     // The argument should be a parenthesized string literal.
-    // The argument to these builtins should be a parenthesized identifier.
-    SourceLocation StartLoc = Tok.getLocation();    
-    bool IsValid = false;
-    bool Value = false;
-    // Read the '('.
-    LexUnexpandedToken(Tok);
-    do {
-      if (Tok.isNot(tok::l_paren)) {
-        Diag(StartLoc, diag::err_warning_check_malformed);
-        break;
-      }
-
-      LexUnexpandedToken(Tok);
+    EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
+      [](Token &Tok, Preprocessor &PP, bool &HasLexedNextToken) -> int {
         std::string WarningName;
         SourceLocation StrStartLoc = Tok.getLocation();
-      if (!FinishLexStringLiteral(Tok, WarningName, "'__has_warning'",
-                                  /*MacroExpansion=*/false)) {
-        // Eat tokens until ')'.
-        while (Tok.isNot(tok::r_paren) && Tok.isNot(tok::eod) &&
-               Tok.isNot(tok::eof))
-          LexUnexpandedToken(Tok);
-        break;
-      }
 
-      // Is the end a ')'?
-      if (!(IsValid = Tok.is(tok::r_paren))) {
-        Diag(StartLoc, diag::err_warning_check_malformed);
-        break;
-      }
+        HasLexedNextToken = Tok.is(tok::string_literal);
+        if (!PP.FinishLexStringLiteral(Tok, WarningName, "'__has_warning'",
+                                       /*MacroExpansion=*/false))
+          return false;
 
-      // FIXME: Should we accept "-R..." flags here, or should that be handled
-      // by a separate __has_remark?
+        // FIXME: Should we accept "-R..." flags here, or should that be
+        // handled by a separate __has_remark?
         if (WarningName.size() < 3 || WarningName[0] != '-' ||
             WarningName[1] != 'W') {
-        Diag(StrStartLoc, diag::warn_has_warning_invalid_option);
-        break;
+          PP.Diag(StrStartLoc, diag::warn_has_warning_invalid_option);
+          return false;
         }
 
         // Finally, check if the warning flags maps to a diagnostic group.
         // We construct a SmallVector here to talk to getDiagnosticIDs().
         // Although we don't use the result, this isn't a hot path, and not
         // worth special casing.
         SmallVector<diag::kind, 10> Diags;
-      Value = !getDiagnostics().getDiagnosticIDs()->
+        return !PP.getDiagnostics().getDiagnosticIDs()->
                 getDiagnosticsInGroup(diag::Flavor::WarningOrError,
                                       WarningName.substr(2), Diags);
-    } while (false);
-
-    if (!IsValid)
-      return;
-    OS << (int)Value;
-    Tok.setKind(tok::numeric_constant);
+      });
   } else if (II == Ident__building_module) {
     // The argument to this builtin should be an identifier. The
     // builtin evaluates to 1 when that identifier names the module we are
     // currently building.
-    OS << (int)EvaluateBuildingModule(Tok, II, *this);
-    Tok.setKind(tok::numeric_constant);
+    EvaluateFeatureLikeBuiltinMacro(OS, Tok, II, *this,
+      [](Token &Tok, Preprocessor &PP, bool &HasLexedNextToken) -> int {
+        IdentifierInfo *II = ExpectFeatureIdentifierInfo(Tok, PP,
+                                       diag::err_expected_id_building_module);
+        return PP.getLangOpts().CompilingModule && II &&
+               (II->getName() == PP.getLangOpts().CurrentModule);
+      });
   } else if (II == Ident__MODULE__) {
     // The current module as an identifier.
     OS << getLangOpts().CurrentModule;
     IdentifierInfo *ModuleII = getIdentifierInfo(getLangOpts().CurrentModule);
     Tok.setIdentifierInfo(ModuleII);
     Tok.setKind(ModuleII->getTokenID());
   } else if (II == Ident__identifier) {
     SourceLocation Loc = Tok.getLocation();

test/Preprocessor/feature_tests.c

 #define HAS_BUILTIN(X) __has_builtin(X)
 #define HAS_FEATURE(X) __has_feature(X)
 
 #if !HAS_BUILTIN(MY_ALIAS_BUILTIN) || !HAS_FEATURE(MY_ALIAS_FEATURE)
 #error Expansion should have occurred
 #endif
 
 #ifdef VERIFY
-// expected-error@+2 {{builtin feature check macro requires a parenthesized identifier}}
-// expected-error@+1 {{expected value in expression}}
+// expected-error@+1 {{builtin feature check macro requires a parenthesized identifier}}
 #if __has_feature('x')
 #endif
+
+// The following are not identifiers:
+_Static_assert(!__is_identifier("string"), "oops");
+_Static_assert(!__is_identifier('c'), "oops");
+_Static_assert(!__is_identifier(123), "oops");
+_Static_assert(!__is_identifier(int), "oops");
+
+// The following are:
+_Static_assert(__is_identifier(abc /* comment */), "oops");
+_Static_assert(__is_identifier /* comment */ (xyz), "oops");
+
+// expected-error@+1 {{too few arguments}}
+#if __is_identifier()
+#endif
+
+// expected-error@+1 {{too many arguments}}
+#if __is_identifier(,())
+#endif
+
+// expected-error@+1 {{missing ')' after 'abc'}} 
+#if __is_identifier(abc xyz) // expected-note {{to match this '('}}
+#endif
+
+// expected-error@+1 {{missing ')' after 'abc'}} 
+#if __is_identifier(abc())   // expected-note {{to match this '('}}
+#endif
+
+// expected-error@+1 {{missing ')' after 'abc'}} 
+#if __is_identifier(abc.)    // expected-note {{to match this '('}}
+#endif
+
+// expected-error@+1 {{'(' not expected after '('}} 
+#if __is_identifier((abc))
+#endif
+
+// expected-error@+1 {{missing '(' after '__is_identifier'}} expected-error@+1 {{expected value}}
+#if __is_identifier
+#endif
+
+// expected-error@+1 {{unterminated}} expected-error@+1 {{expected value}}
+#if __is_identifier(
+#endif
+
 #endif

test/Preprocessor/invalid-__has_warning1.c

 // RUN: %clang_cc1 -verify %s
 
 // These must be the last lines in this test.
-// expected-error@+1{{expected string literal}} expected-error@+1 2{{expected}}
+// expected-error@+1{{unterminated}} expected-error@+1 2{{expected}}
 int i = __has_warning(

test/Preprocessor/invalid-__has_warning2.c

 // RUN: %clang_cc1 -verify %s
 
 // These must be the last lines in this test.
-// expected-error@+1{{expected string literal}} expected-error@+1{{expected}}
+// expected-error@+1{{too few arguments}}
 int i = __has_warning();

test/Preprocessor/warning_tests.c

 // RUN: %clang_cc1 -fsyntax-only %s -verify
 #ifndef __has_warning
 #error Should have __has_warning
 #endif
 
 #if __has_warning("not valid") // expected-warning {{__has_warning expected option name}}
 #endif
 
 // expected-warning@+2 {{Should have -Wparentheses}}
 #if __has_warning("-Wparentheses")
 #warning Should have -Wparentheses
 #endif
 
 // expected-error@+2 {{expected string literal in '__has_warning'}}
-// expected-error@+1 {{expected value in expression}}
+// expected-error@+1 {{missing ')'}} expected-note@+1 {{match}}
 #if __has_warning(-Wfoo)
 #endif
 
 // expected-warning@+3 {{Not a valid warning flag}}
 #if __has_warning("-Wnot-a-valid-warning-flag-at-all")
 #else
 #warning Not a valid warning flag
 #endif
 
-// expected-error@+2 {{builtin warning check macro requires a parenthesized string}}
-// expected-error@+1 {{invalid token}}
+// expected-error@+1 {{missing '(' after '__has_warning'}}
 #if __has_warning "not valid"
 #endif
 
 // Macro expansion does not occur in the parameter to __has_warning
 // (as is also expected behaviour for ordinary macros), so the
 // following should not expand:
 
 #define MY_ALIAS "-Wparentheses"
 
-// expected-error@+1 2{{expected}}
+// expected-error@+1 {{expected}}
 #if __has_warning(MY_ALIAS)
 #error Alias expansion not allowed
 #endif
 
 // But deferring should expand:
 #define HAS_WARNING(X) __has_warning(X)
 
 #if !HAS_WARNING(MY_ALIAS)
 #error Expansion should have occurred
 #endif