Re: [PATCH] D15636: Reduce false positives in printf/scanf format checker

2016-02-03 Thread Andy Gibbs via cfe-commits 
AndyG added a comment.

Thoughts?  Am I good to go?

Cheers,
Andy


http://reviews.llvm.org/D15636



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15636: Reduce false positives in printf/scanf format checker

2016-02-03 Thread Andy Gibbs via cfe-commits 
AndyG added a comment.

In your case, the first string would be highlighted only.  Yes, I see what you 
mean.  Is it possible to have multiple ranges for the diagnostic?  By which I 
mean, to produce the following:

  test.cpp:x:y: warning: data argument not used by format string 
[-Wformat-extra-args]
  printf(condition ? "first message: %d" : "second message: %d", 5, 10);
 ~~~    ^

If so, I think the diagnostic should still track just the strings utilising the 
most arguments, so this would be the case with three messages, missing out the 
middle string since it is already using a sub-set of the arguments anyway:

  test.cpp:x:y: warning: data argument not used by format string 
[-Wformat-extra-args]
  printf(a ? "%d %d" : b ? "%d" : "%d %d", 5, 10, 20);
  ~~  ^

If you think this isn't sufficient, then my suggestion would be to make the 
range of the diagnostic the complete format expression, and let the programmer 
assess it themselves.


http://reviews.llvm.org/D15636



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15636: Reduce false positives in printf/scanf format checker

2016-02-10 Thread Andy Gibbs via cfe-commits 
AndyG removed reviewers: dblaikie, rsmith.
AndyG updated this revision to Diff 47426.
AndyG added a comment.

All strings matching the highest uncovered argument are now highlighted in the 
diagnostic.


http://reviews.llvm.org/D15636

Files:
  include/clang/Sema/Sema.h
  lib/Sema/SemaChecking.cpp
  test/Sema/format-strings.c

Index: test/Sema/format-strings.c
===
--- test/Sema/format-strings.c
+++ test/Sema/format-strings.c
@@ -86,6 +86,19 @@
   printf(i == 0 ? (i == 1 ? "yes" : "no") : "dont know"); // no-warning
   printf(i == 0 ? (i == 1 ? s : "no") : "dont know"); // expected-warning{{format string is not a string literal}}
   printf("yes" ?: "no %d", 1); // expected-warning{{data argument not used by format string}}
+  printf(0 ? "yes %s" : "no %d", 1); // no-warning
+  printf(0 ? "yes %d" : "no %s", 1); // expected-warning{{format specifies type 'char *'}}
+
+  printf(0 ? "yes" : "no %d", 1); // no-warning
+  printf(0 ? "yes %d" : "no", 1); // expected-warning{{data argument not used by format string}}
+  printf(1 ? "yes" : "no %d", 1); // expected-warning{{data argument not used by format string}}
+  printf(1 ? "yes %d" : "no", 1); // no-warning
+  printf(i ? "yes" : "no %d", 1); // no-warning
+  printf(i ? "yes %s" : "no %d", 1); // expected-warning{{format specifies type 'char *'}}
+  printf(i ? "yes" : "no %d", 1, 2); // expected-warning{{data argument not used by format string}}
+
+  printf(i ? "%*s" : "-", i, s); // no-warning
+  printf(i ? "yes" : 0 ? "no %*d" : "dont know %d", 1, 2); // expected-warning{{data argument not used by format string}}
 }
 
 void check_writeback_specifier()
@@ -519,7 +532,7 @@
 
   // Make sure that the "format string is defined here" note is not emitted
   // when the original string is within the argument expression.
-  printf(1 ? "yes %d" : "no %d"); // expected-warning 2{{more '%' conversions than data arguments}}
+  printf(1 ? "yes %d" : "no %d"); // expected-warning{{more '%' conversions than data arguments}}
 
   const char kFormat17[] = "%hu"; // expected-note{{format string is defined here}}}
   printf(kFormat17, (int[]){0}); // expected-warning{{format specifies type 'unsigned short' but the argument}}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -3233,6 +3233,16 @@
 }
 
 namespace {
+struct UncoveredArgHandler {
+  enum { Unknown = -1, AllCovered = -2 };
+  signed FirstUncoveredArg;
+  SmallVector DiagnosticExprs;
+
+  UncoveredArgHandler() : FirstUncoveredArg(Unknown) { }
+  void Diagnose(Sema &S, bool IsFunctionCall, const Expr *ArgExpr);
+  void Update(signed NewFirstUncoveredArg, const Expr *StrExpr);
+};
+
 enum StringLiteralCheckType {
   SLCT_NotALiteral,
   SLCT_UncheckedLiteral,
@@ -3249,7 +3259,8 @@
   bool HasVAListArg, unsigned format_idx,
   unsigned firstDataArg, Sema::FormatStringType Type,
   Sema::VariadicCallType CallType, bool InFunctionCall,
-  llvm::SmallBitVector &CheckedVarArgs) {
+  llvm::SmallBitVector &CheckedVarArgs,
+  UncoveredArgHandler &UncoveredArg) {
  tryAgain:
   if (E->isTypeDependent() || E->isValueDependent())
 return SLCT_NotALiteral;
@@ -3270,17 +3281,39 @@
 // completely checked only if both sub-expressions were checked.
 const AbstractConditionalOperator *C =
 cast(E);
-StringLiteralCheckType Left =
-checkFormatStringExpr(S, C->getTrueExpr(), Args,
-  HasVAListArg, format_idx, firstDataArg,
-  Type, CallType, InFunctionCall, CheckedVarArgs);
-if (Left == SLCT_NotALiteral)
-  return SLCT_NotALiteral;
+
+// Determine whether it is necessary to check both sub-expressions, for
+// example, because the condition expression is a constant that can be
+// evaluated at compile time.
+bool CheckLeft = true, CheckRight = true;
+
+bool Cond;
+if (C->getCond()->EvaluateAsBooleanCondition(Cond, S.getASTContext())) {
+  if (Cond)
+CheckRight = false;
+  else
+CheckLeft = false;
+}
+
+StringLiteralCheckType Left;
+if (!CheckLeft)
+  Left = SLCT_UncheckedLiteral;
+else {
+  Left = checkFormatStringExpr(S, C->getTrueExpr(), Args,
+   HasVAListArg, format_idx, firstDataArg,
+   Type, CallType, InFunctionCall,
+   CheckedVarArgs, UncoveredArg);
+  if (Left == SLCT_NotALiteral || !CheckRight)
+return Left;
+}
+
 StringLiteralCheckType Right =
 checkFormatStringExpr(S, C->getFalseExpr(), Args,
   HasVAListArg, format_idx, firstDataArg,
-  Type, CallType, InFunctionCall, CheckedVarArgs);
-retu

Re: [PATCH] D17149: Consolidate and improve the handling of built-in feature-like macros

2016-02-11 Thread Andy Gibbs via cfe-commits 
AndyG added a comment.

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


http://reviews.llvm.org/D17149



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17149: Consolidate and improve the handling of built-in feature-like macros

2016-02-11 Thread Andy Gibbs via cfe-commits 
AndyG added a comment.

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)


http://reviews.llvm.org/D17149



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15636: Reduce false positives in printf/scanf format checker

2016-02-15 Thread Andy Gibbs via cfe-commits 
AndyG added a comment.

Richard, are you happy with this latest version?  Can I proceed to commit it?

Thanks.


http://reviews.llvm.org/D15636



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17149: Consolidate and improve the handling of built-in feature-like macros

2016-02-18 Thread Andy Gibbs via cfe-commits 
AndyG added a comment.

Bump :o)


http://reviews.llvm.org/D17149



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r261522 - Make Sema::CheckFormatString a static function inside SemaChecking.cpp

2016-02-22 Thread Andy Gibbs via cfe-commits 
Author: andyg
Date: Mon Feb 22 07:00:43 2016
New Revision: 261522

URL: http://llvm.org/viewvc/llvm-project?rev=261522&view=rev
Log:
Make Sema::CheckFormatString a static function inside SemaChecking.cpp

No functionality change.  Change at the request of Richard Trieu, see
http://reviews.llvm.org/D15636#357858.

Modified:
cfe/trunk/include/clang/Sema/Sema.h
cfe/trunk/lib/Sema/SemaChecking.cpp

Modified: cfe/trunk/include/clang/Sema/Sema.h
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Sema/Sema.h?rev=261522&r1=261521&r2=261522&view=diff
==
--- cfe/trunk/include/clang/Sema/Sema.h (original)
+++ cfe/trunk/include/clang/Sema/Sema.h Mon Feb 22 07:00:43 2016
@@ -9111,13 +9111,6 @@ public:
   };
   static FormatStringType GetFormatStringType(const FormatAttr *Format);
 
-  void CheckFormatString(const StringLiteral *FExpr, const Expr 
*OrigFormatExpr,
- ArrayRef Args, bool HasVAListArg,
- unsigned format_idx, unsigned firstDataArg,
- FormatStringType Type, bool inFunctionCall,
- VariadicCallType CallType,
- llvm::SmallBitVector &CheckedVarArgs);
-  
   bool FormatStringHasSArg(const StringLiteral *FExpr);
   
   static bool GetFormatNSStringIdx(const FormatAttr *Format, unsigned &Idx);

Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=261522&r1=261521&r2=261522&view=diff
==
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Mon Feb 22 07:00:43 2016
@@ -3265,6 +3265,16 @@ enum StringLiteralCheckType {
 };
 } // end anonymous namespace
 
+static void CheckFormatString(Sema &S, const StringLiteral *FExpr,
+  const Expr *OrigFormatExpr,
+  ArrayRef Args,
+  bool HasVAListArg, unsigned format_idx,
+  unsigned firstDataArg,
+  Sema::FormatStringType Type,
+  bool inFunctionCall,
+  Sema::VariadicCallType CallType,
+  llvm::SmallBitVector &CheckedVarArgs);
+
 // Determine if an expression is a string literal or constant string.
 // If this function returns false on the arguments to a function expecting a
 // format string, we will usually need to emit a warning.
@@ -3437,8 +3447,9 @@ checkFormatStringExpr(Sema &S, const Exp
   StrE = cast(E);
 
 if (StrE) {
-  S.CheckFormatString(StrE, E, Args, HasVAListArg, format_idx, 
firstDataArg,
-  Type, InFunctionCall, CallType, CheckedVarArgs);
+  CheckFormatString(S, StrE, E, Args, HasVAListArg, format_idx,
+firstDataArg, Type, InFunctionCall, CallType,
+CheckedVarArgs);
   return SLCT_CheckedLiteral;
 }
 
@@ -4906,27 +4917,30 @@ bool CheckScanfHandler::HandleScanfSpeci
   return true;
 }
 
-void Sema::CheckFormatString(const StringLiteral *FExpr,
- const Expr *OrigFormatExpr,
- ArrayRef Args,
- bool HasVAListArg, unsigned format_idx,
- unsigned firstDataArg, FormatStringType Type,
- bool inFunctionCall, VariadicCallType CallType,
- llvm::SmallBitVector &CheckedVarArgs) {
+static void CheckFormatString(Sema &S, const StringLiteral *FExpr,
+  const Expr *OrigFormatExpr,
+  ArrayRef Args,
+  bool HasVAListArg, unsigned format_idx,
+  unsigned firstDataArg,
+  Sema::FormatStringType Type,
+  bool inFunctionCall,
+  Sema::VariadicCallType CallType,
+  llvm::SmallBitVector &CheckedVarArgs) {
   // CHECK: is the format string a wide literal?
   if (!FExpr->isAscii() && !FExpr->isUTF8()) {
 CheckFormatHandler::EmitFormatDiagnostic(
-  *this, inFunctionCall, Args[format_idx],
-  PDiag(diag::warn_format_string_is_wide_literal), FExpr->getLocStart(),
+  S, inFunctionCall, Args[format_idx],
+  S.PDiag(diag::warn_format_string_is_wide_literal), FExpr->getLocStart(),
   /*IsStringLocation*/true, OrigFormatExpr->getSourceRange());
 return;
   }
-  
+
   // Str - The format string.  NOTE: this is NOT null-terminated!
   StringRef StrRef = FExpr->getString();
   const char *Str = StrRef.data();
   // Account for cases where the string literal is truncated in a declaration.
-  const ConstantArrayType *T = 
Context.getAsConstantArrayType(FExpr->getType());
+

Re: [PATCH] D15636: Reduce false positives in printf/scanf format checker

2016-02-22 Thread Andy Gibbs via cfe-commits 
AndyG marked 7 inline comments as done.
AndyG added a comment.

I've removed CheckFormatString from Sema.h and make it a static function inside 
SemaChecking.cpp in r261522.  I'll submit a new diff here with the remaining 
requested changes for this patch when I have a moment.


http://reviews.llvm.org/D15636



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15636: Reduce false positives in printf/scanf format checker

2016-02-22 Thread Andy Gibbs via cfe-commits 
AndyG marked 11 inline comments as done.
AndyG added a comment.

Revised patch coming shortly...



Comment at: lib/Sema/SemaChecking.cpp:3603
@@ -3554,3 +3602,3 @@
 
-  void DoneProcessing();
+  void DoneProcessing(signed &FirstUncoveredArg);
 

rtrieu wrote:
> Don't change the call signature here, leave it as a zero argument call.  Have 
> another function return the first uncovered argument.
DoneProcessing is that function!  To move the logic into another function would 
leave nothing behind :o)

Passing UncoveredArgHandler& into CheckFormatHandler allows this to be worked 
around.


Comment at: lib/Sema/SemaChecking.cpp:3840-3892
@@ -3796,5 +3839,55 @@
+
+void UncoveredArgHandler::Update(signed NewFirstUncoveredArg,
+ const Expr *StrExpr) {
+  // Don't update if a previous string covers all arguments.
+  if (FirstUncoveredArg == AllCovered)
+return;
+
+  switch (NewFirstUncoveredArg) {
+case Unknown:
+  // The string was not fully analysed.  Do nothing.
+  break;
+
+case UncoveredArgHandler::AllCovered:
+  // A string has been found with all arguments covered, so clear out
+  // the diagnostics.
+  FirstUncoveredArg = AllCovered;
+  DiagnosticExprs.clear();
+  break;
+
+default:
+  // UncoveredArgHandler tracks the highest uncovered argument index
+  // and with it all the strings that match this index.
+  if (NewFirstUncoveredArg == FirstUncoveredArg)
+DiagnosticExprs.push_back(StrExpr);
+  else if (NewFirstUncoveredArg > FirstUncoveredArg) {
+DiagnosticExprs.clear();
+DiagnosticExprs.push_back(StrExpr);
+FirstUncoveredArg = NewFirstUncoveredArg;
   }
-}
+  break;
   }
 }
 
+void UncoveredArgHandler::Diagnose(Sema &S, bool IsFunctionCall,
+   const Expr *ArgExpr) {
+  assert(FirstUncoveredArg >= 0 && DiagnosticExprs.size() > 0 &&
+ "Invalid state");
+
+  if (!ArgExpr)
+return;
+
+  SourceLocation Loc = ArgExpr->getLocStart();
+
+  if (S.getSourceManager().isInSystemMacro(Loc))
+return;
+
+  PartialDiagnostic PDiag = S.PDiag(diag::warn_printf_data_arg_not_used);
+  for (unsigned i = 1; i < DiagnosticExprs.size(); ++i)
+PDiag << DiagnosticExprs[i]->getSourceRange();
+
+  CheckFormatHandler::EmitFormatDiagnostic(
+  S, IsFunctionCall, DiagnosticExprs[0],
+  PDiag, Loc, /*IsStringLocation*/false,
+  DiagnosticExprs[0]->getSourceRange());
+}

rtrieu wrote:
> Move these two functions up to where the class is defined.
UncoveredArgHandler::Update has been moved up, but 
UncoveredArgHandler::Diagnose has been left since it has a dependency on 
CheckFormatHandler.


Comment at: lib/Sema/SemaChecking.cpp:3886-3887
@@ +3885,4 @@
+  PartialDiagnostic PDiag = S.PDiag(diag::warn_printf_data_arg_not_used);
+  for (unsigned i = 1; i < DiagnosticExprs.size(); ++i)
+PDiag << DiagnosticExprs[i]->getSourceRange();
+

rtrieu wrote:
> Use a range-based for-loop here.
Except that it is iterating from the second item in the list.  I don't think 
there is an easier way to do it.


http://reviews.llvm.org/D15636



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15636: Reduce false positives in printf/scanf format checker

2016-02-22 Thread Andy Gibbs via cfe-commits 
AndyG updated this revision to Diff 48678.
AndyG marked 2 inline comments as done.
AndyG added a comment.

Patch additionally re-based off r261522.


http://reviews.llvm.org/D15636

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/format-strings-scanf.c
  test/Sema/format-strings.c

Index: test/Sema/format-strings.c
===
--- test/Sema/format-strings.c
+++ test/Sema/format-strings.c
@@ -46,6 +46,9 @@
 
   vscanf(s, ap); // expected-warning {{format string is not a string literal}}
 
+  const char *const fmt = "%d"; // FIXME -- defined here
+  printf(fmt, 1, 2); // expected-warning{{data argument not used}}
+
   // rdar://6079877
   printf("abc"
  "%*d", 1, 1); // no-warning
@@ -86,6 +89,19 @@
   printf(i == 0 ? (i == 1 ? "yes" : "no") : "dont know"); // no-warning
   printf(i == 0 ? (i == 1 ? s : "no") : "dont know"); // expected-warning{{format string is not a string literal}}
   printf("yes" ?: "no %d", 1); // expected-warning{{data argument not used by format string}}
+  printf(0 ? "yes %s" : "no %d", 1); // no-warning
+  printf(0 ? "yes %d" : "no %s", 1); // expected-warning{{format specifies type 'char *'}}
+
+  printf(0 ? "yes" : "no %d", 1); // no-warning
+  printf(0 ? "yes %d" : "no", 1); // expected-warning{{data argument not used by format string}}
+  printf(1 ? "yes" : "no %d", 1); // expected-warning{{data argument not used by format string}}
+  printf(1 ? "yes %d" : "no", 1); // no-warning
+  printf(i ? "yes" : "no %d", 1); // no-warning
+  printf(i ? "yes %s" : "no %d", 1); // expected-warning{{format specifies type 'char *'}}
+  printf(i ? "yes" : "no %d", 1, 2); // expected-warning{{data argument not used by format string}}
+
+  printf(i ? "%*s" : "-", i, s); // no-warning
+  printf(i ? "yes" : 0 ? "no %*d" : "dont know %d", 1, 2); // expected-warning{{data argument not used by format string}}
 }
 
 void check_writeback_specifier()
@@ -519,7 +535,7 @@
 
   // Make sure that the "format string is defined here" note is not emitted
   // when the original string is within the argument expression.
-  printf(1 ? "yes %d" : "no %d"); // expected-warning 2{{more '%' conversions than data arguments}}
+  printf(1 ? "yes %d" : "no %d"); // expected-warning{{more '%' conversions than data arguments}}
 
   const char kFormat17[] = "%hu"; // expected-note{{format string is defined here}}}
   printf(kFormat17, (int[]){0}); // expected-warning{{format specifies type 'unsigned short' but the argument}}
Index: test/Sema/format-strings-scanf.c
===
--- test/Sema/format-strings-scanf.c
+++ test/Sema/format-strings-scanf.c
@@ -18,7 +18,7 @@
 int vsscanf(const char * restrict, const char * restrict, va_list);
 
 void test(const char *s, int *i) {
-  scanf(s, i); // expected-warning{{ormat string is not a string literal}}
+  scanf(s, i); // expected-warning{{format string is not a string literal}}
   scanf("%0d", i); // expected-warning{{zero field width in scanf format string is unused}}
   scanf("%00d", i); // expected-warning{{zero field width in scanf format string is unused}}
   scanf("%d%[asdfasdfd", i, s); // expected-warning{{no closing ']' for '%[' in scanf format string}}
@@ -171,3 +171,15 @@
   scanf("%d", (ip_t)0); // No warning.
   scanf("%d", (cip_t)0); // expected-warning{{format specifies type 'int *' but the argument has type 'cip_t' (aka 'const int *')}}
 }
+
+void check_conditional_literal(char *s, int *i) {
+  scanf(0 ? "%s" : "%d", i); // no warning
+  scanf(1 ? "%s" : "%d", i); // expected-warning{{format specifies type 'char *'}}
+  scanf(0 ? "%d %d" : "%d", i); // no warning
+  scanf(1 ? "%d %d" : "%d", i); // expected-warning{{more '%' conversions than data arguments}}
+  scanf(0 ? "%d %d" : "%d", i, s); // expected-warning{{data argument not used}}
+  scanf(1 ? "%d %s" : "%d", i, s); // no warning
+  scanf(i ? "%d %s" : "%d", i, s); // no warning
+  scanf(i ? "%d" : "%d", i, s); // expected-warning{{data argument not used}}
+  scanf(i ? "%s" : "%d", s); // expected-warning{{format specifies type 'int *'}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -3258,6 +3258,50 @@
 }
 
 namespace {
+class UncoveredArgHandler {
+  enum { Unknown = -1, AllCovered = -2 };
+  signed FirstUncoveredArg;
+  SmallVector DiagnosticExprs;
+
+public:
+  UncoveredArgHandler() : FirstUncoveredArg(Unknown) { }
+
+  bool hasUncoveredArg() const {
+return (FirstUncoveredArg >= 0);
+  }
+
+  unsigned getUncoveredArg() const {
+assert(hasUncoveredArg() && "no uncovered argument");
+return FirstUncoveredArg;
+  }
+
+  void Update(signed NewFirstUncoveredArg, const Expr *StrExpr) {
+// Don't update if a previous string covers all arguments.
+if (FirstUncoveredArg == AllCovered)
+  return;
+
+if (NewFirstUncoveredArg < 0) {
+  // A string

Re: [PATCH] D15636: Reduce false positives in printf/scanf format checker

2016-02-22 Thread Andy Gibbs via cfe-commits 
AndyG added inline comments.


Comment at: lib/Sema/SemaChecking.cpp:3905
@@ -3822,14 +3904,3 @@
 CoveredArgs.flip();
-signed notCoveredArg = CoveredArgs.find_first();
-if (notCoveredArg >= 0) {
-  assert((unsigned)notCoveredArg < NumDataArgs);
-  if (const Expr *E = getDataArg((unsigned) notCoveredArg)) {
-SourceLocation Loc = E->getLocStart();
-if (!S.getSourceManager().isInSystemMacro(Loc)) {
-  EmitFormatDiagnostic(S.PDiag(diag::warn_printf_data_arg_not_used),
-   Loc, /*IsStringLocation*/false,
-   getFormatStringRange());
-}
-  }
-}
+UncoveredArg.Update(CoveredArgs.find_first(), FExpr);
   }

Rather than `FExpr`, I think this should be `OrigFormatExpr`?


http://reviews.llvm.org/D15636



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17149: Consolidate and improve the handling of built-in feature-like macros

2016-02-25 Thread Andy Gibbs via cfe-commits 
AndyG added a comment.

Second bump :o)


http://reviews.llvm.org/D17149



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15636: Reduce false positives in printf/scanf format checker

2016-02-25 Thread Andy Gibbs via cfe-commits 
AndyG updated this revision to Diff 49058.
AndyG marked 5 inline comments as done.
AndyG added a comment.

Updated patch according to the comments made.  Also spotted a corner-case where 
a double-diagnostic was produced, for example in the following where one string 
has too few arguments and the other string too many:

  printf(minimal ? "%i\n" : "%i %s %s\n", code, msg);

Hitherto, this produced two diagnostics: the first (correct) diagnostic that 
more '%' conversions exist than data arguments, but then a second diagnostic 
regarding uncovered arguments.  The second diagnostic is now suppressed since 
the format string that has too many does also cover all arguments.


http://reviews.llvm.org/D15636

Files:
  lib/Sema/SemaChecking.cpp
  test/Sema/format-strings-scanf.c
  test/Sema/format-strings.c

Index: test/Sema/format-strings.c
===
--- test/Sema/format-strings.c
+++ test/Sema/format-strings.c
@@ -46,6 +46,9 @@
 
   vscanf(s, ap); // expected-warning {{format string is not a string literal}}
 
+  const char *const fmt = "%d"; // FIXME -- defined here
+  printf(fmt, 1, 2); // expected-warning{{data argument not used}}
+
   // rdar://6079877
   printf("abc"
  "%*d", 1, 1); // no-warning
@@ -86,6 +89,20 @@
   printf(i == 0 ? (i == 1 ? "yes" : "no") : "dont know"); // no-warning
   printf(i == 0 ? (i == 1 ? s : "no") : "dont know"); // expected-warning{{format string is not a string literal}}
   printf("yes" ?: "no %d", 1); // expected-warning{{data argument not used by format string}}
+  printf(0 ? "yes %s" : "no %d", 1); // no-warning
+  printf(0 ? "yes %d" : "no %s", 1); // expected-warning{{format specifies type 'char *'}}
+
+  printf(0 ? "yes" : "no %d", 1); // no-warning
+  printf(0 ? "yes %d" : "no", 1); // expected-warning{{data argument not used by format string}}
+  printf(1 ? "yes" : "no %d", 1); // expected-warning{{data argument not used by format string}}
+  printf(1 ? "yes %d" : "no", 1); // no-warning
+  printf(i ? "yes" : "no %d", 1); // no-warning
+  printf(i ? "yes %s" : "no %d", 1); // expected-warning{{format specifies type 'char *'}}
+  printf(i ? "yes" : "no %d", 1, 2); // expected-warning{{data argument not used by format string}}
+
+  printf(i ? "%*s" : "-", i, s); // no-warning
+  printf(i ? "yes" : 0 ? "no %*d" : "dont know %d", 1, 2); // expected-warning{{data argument not used by format string}}
+  printf(i ? "%i\n" : "%i %s %s\n", i, s); // expected-warning{{more '%' conversions than data arguments}}
 }
 
 void check_writeback_specifier()
@@ -519,7 +536,7 @@
 
   // Make sure that the "format string is defined here" note is not emitted
   // when the original string is within the argument expression.
-  printf(1 ? "yes %d" : "no %d"); // expected-warning 2{{more '%' conversions than data arguments}}
+  printf(1 ? "yes %d" : "no %d"); // expected-warning{{more '%' conversions than data arguments}}
 
   const char kFormat17[] = "%hu"; // expected-note{{format string is defined here}}}
   printf(kFormat17, (int[]){0}); // expected-warning{{format specifies type 'unsigned short' but the argument}}
Index: test/Sema/format-strings-scanf.c
===
--- test/Sema/format-strings-scanf.c
+++ test/Sema/format-strings-scanf.c
@@ -18,7 +18,7 @@
 int vsscanf(const char * restrict, const char * restrict, va_list);
 
 void test(const char *s, int *i) {
-  scanf(s, i); // expected-warning{{ormat string is not a string literal}}
+  scanf(s, i); // expected-warning{{format string is not a string literal}}
   scanf("%0d", i); // expected-warning{{zero field width in scanf format string is unused}}
   scanf("%00d", i); // expected-warning{{zero field width in scanf format string is unused}}
   scanf("%d%[asdfasdfd", i, s); // expected-warning{{no closing ']' for '%[' in scanf format string}}
@@ -171,3 +171,15 @@
   scanf("%d", (ip_t)0); // No warning.
   scanf("%d", (cip_t)0); // expected-warning{{format specifies type 'int *' but the argument has type 'cip_t' (aka 'const int *')}}
 }
+
+void check_conditional_literal(char *s, int *i) {
+  scanf(0 ? "%s" : "%d", i); // no warning
+  scanf(1 ? "%s" : "%d", i); // expected-warning{{format specifies type 'char *'}}
+  scanf(0 ? "%d %d" : "%d", i); // no warning
+  scanf(1 ? "%d %d" : "%d", i); // expected-warning{{more '%' conversions than data arguments}}
+  scanf(0 ? "%d %d" : "%d", i, s); // expected-warning{{data argument not used}}
+  scanf(1 ? "%d %s" : "%d", i, s); // no warning
+  scanf(i ? "%d %s" : "%d", i, s); // no warning
+  scanf(i ? "%d" : "%d", i, s); // expected-warning{{data argument not used}}
+  scanf(i ? "%s" : "%d", s); // expected-warning{{format specifies type 'int *'}}
+}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -3258,6 +3258,51 @@
 }
 
 namespace {
+class UncoveredArgHandl

Re: [PATCH] D15636: Reduce false positives in printf/scanf format checker

2016-02-25 Thread Andy Gibbs via cfe-commits 
AndyG added inline comments.


Comment at: lib/Sema/SemaChecking.cpp:3923-3924
@@ +3922,4 @@
+  PartialDiagnostic PDiag = S.PDiag(diag::warn_printf_data_arg_not_used);
+  for (unsigned i = 1; i < DiagnosticExprs.size(); ++i)
+PDiag << DiagnosticExprs[i]->getSourceRange();
+

rtrieu wrote:
> That raises the question why the first element is different from the rest.
I've put this as a range-based for-loop as requested.  The reason it wasn't 
before is that one expression is passed through the call to 
EmitFormatDiagnostic.  Ultimately, I think EmitFormatDiagnostic needs to be 
reworked to support multiple format strings - and this will probably resolve 
the FIXME relating to the missing diagnostic note in the tests.


http://reviews.llvm.org/D15636



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

r262025 - Reduce false positives in printf/scanf format checker

2016-02-26 Thread Andy Gibbs via cfe-commits 
Author: andyg
Date: Fri Feb 26 09:35:16 2016
New Revision: 262025

URL: http://llvm.org/viewvc/llvm-project?rev=262025&view=rev
Log:
Reduce false positives in printf/scanf format checker

Summary:
The printf/scanf format checker is a little over-zealous in handling the 
conditional operator.  This patch reduces work by not checking code-paths that 
are never used and reduces false positives regarding uncovered arguments, for 
example in the code fragment:

printf(minimal ? "%i\n" : "%i: %s\n", code, msg);

Reviewers: rtrieu

Subscribers: cfe-commits

Differential Revision: http://reviews.llvm.org/D15636

Modified:
cfe/trunk/lib/Sema/SemaChecking.cpp
cfe/trunk/test/Sema/format-strings-scanf.c
cfe/trunk/test/Sema/format-strings.c

Modified: cfe/trunk/lib/Sema/SemaChecking.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Sema/SemaChecking.cpp?rev=262025&r1=262024&r2=262025&view=diff
==
--- cfe/trunk/lib/Sema/SemaChecking.cpp (original)
+++ cfe/trunk/lib/Sema/SemaChecking.cpp Fri Feb 26 09:35:16 2016
@@ -3256,6 +3256,51 @@ bool Sema::SemaBuiltinSetjmp(CallExpr *T
 }
 
 namespace {
+class UncoveredArgHandler {
+  enum { Unknown = -1, AllCovered = -2 };
+  signed FirstUncoveredArg;
+  SmallVector DiagnosticExprs;
+
+public:
+  UncoveredArgHandler() : FirstUncoveredArg(Unknown) { }
+
+  bool hasUncoveredArg() const {
+return (FirstUncoveredArg >= 0);
+  }
+
+  unsigned getUncoveredArg() const {
+assert(hasUncoveredArg() && "no uncovered argument");
+return FirstUncoveredArg;
+  }
+
+  void setAllCovered() {
+// A string has been found with all arguments covered, so clear out
+// the diagnostics.
+DiagnosticExprs.clear();
+FirstUncoveredArg = AllCovered;
+  }
+
+  void Update(signed NewFirstUncoveredArg, const Expr *StrExpr) {
+assert(NewFirstUncoveredArg >= 0 && "Outside range");
+
+// Don't update if a previous string covers all arguments.
+if (FirstUncoveredArg == AllCovered)
+  return;
+
+// UncoveredArgHandler tracks the highest uncovered argument index
+// and with it all the strings that match this index.
+if (NewFirstUncoveredArg == FirstUncoveredArg)
+  DiagnosticExprs.push_back(StrExpr);
+else if (NewFirstUncoveredArg > FirstUncoveredArg) {
+  DiagnosticExprs.clear();
+  DiagnosticExprs.push_back(StrExpr);
+  FirstUncoveredArg = NewFirstUncoveredArg;
+}
+  }
+
+  void Diagnose(Sema &S, bool IsFunctionCall, const Expr *ArgExpr);
+};
+
 enum StringLiteralCheckType {
   SLCT_NotALiteral,
   SLCT_UncheckedLiteral,
@@ -3271,7 +3316,8 @@ static void CheckFormatString(Sema &S, c
   Sema::FormatStringType Type,
   bool inFunctionCall,
   Sema::VariadicCallType CallType,
-  llvm::SmallBitVector &CheckedVarArgs);
+  llvm::SmallBitVector &CheckedVarArgs,
+  UncoveredArgHandler &UncoveredArg);
 
 // Determine if an expression is a string literal or constant string.
 // If this function returns false on the arguments to a function expecting a
@@ -3282,7 +3328,8 @@ checkFormatStringExpr(Sema &S, const Exp
   bool HasVAListArg, unsigned format_idx,
   unsigned firstDataArg, Sema::FormatStringType Type,
   Sema::VariadicCallType CallType, bool InFunctionCall,
-  llvm::SmallBitVector &CheckedVarArgs) {
+  llvm::SmallBitVector &CheckedVarArgs,
+  UncoveredArgHandler &UncoveredArg) {
  tryAgain:
   if (E->isTypeDependent() || E->isValueDependent())
 return SLCT_NotALiteral;
@@ -3303,17 +3350,39 @@ checkFormatStringExpr(Sema &S, const Exp
 // completely checked only if both sub-expressions were checked.
 const AbstractConditionalOperator *C =
 cast(E);
-StringLiteralCheckType Left =
-checkFormatStringExpr(S, C->getTrueExpr(), Args,
-  HasVAListArg, format_idx, firstDataArg,
-  Type, CallType, InFunctionCall, CheckedVarArgs);
-if (Left == SLCT_NotALiteral)
-  return SLCT_NotALiteral;
+
+// Determine whether it is necessary to check both sub-expressions, for
+// example, because the condition expression is a constant that can be
+// evaluated at compile time.
+bool CheckLeft = true, CheckRight = true;
+
+bool Cond;
+if (C->getCond()->EvaluateAsBooleanCondition(Cond, S.getASTContext())) {
+  if (Cond)
+CheckRight = false;
+  else
+CheckLeft = false;
+}
+
+StringLiteralCheckType Left;
+if (!CheckLeft)
+  Left = SLCT_UncheckedLiteral;
+else {
+  Left = checkFormatStringExpr(S, C->getTrueExpr(), Args,
+   HasVAListArg, format_idx, firstDataArg,
+

r265177 - Diagnose missing macro argument following charize operator.

2016-04-01 Thread Andy Gibbs via cfe-commits 
Author: andyg
Date: Fri Apr  1 14:02:20 2016
New Revision: 265177

URL: http://llvm.org/viewvc/llvm-project?rev=265177&view=rev
Log:
Diagnose missing macro argument following charize operator.

For completeness, add a test-case for the equivalent stringize operator
diagnostic too.

Modified:
cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td
cfe/trunk/lib/Lex/PPDirectives.cpp
cfe/trunk/test/Parser/MicrosoftExtensions.c
cfe/trunk/test/Preprocessor/stringize_misc.c

Modified: cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td?rev=265177&r1=265176&r2=265177&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td Fri Apr  1 14:02:20 2016
@@ -387,7 +387,7 @@ def err_pp_expected_comma_in_arg_list :
 def err_pp_duplicate_name_in_arg_list : Error<
   "duplicate macro parameter name %0">;
 def err_pp_stringize_not_parameter : Error<
-  "'#' is not followed by a macro parameter">;
+  "'%select{#|#@}0' is not followed by a macro parameter">;
 def err_pp_malformed_ident : Error<"invalid #ident directive">;
 def err_pp_unterminated_conditional : Error<
   "unterminated conditional directive">;

Modified: cfe/trunk/lib/Lex/PPDirectives.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPDirectives.cpp?rev=265177&r1=265176&r2=265177&view=diff
==
--- cfe/trunk/lib/Lex/PPDirectives.cpp (original)
+++ cfe/trunk/lib/Lex/PPDirectives.cpp Fri Apr  1 14:02:20 2016
@@ -2151,7 +2151,7 @@ void Preprocessor::HandleDefineDirective
 while (Tok.isNot(tok::eod)) {
   LastTok = Tok;
 
-  if (Tok.isNot(tok::hash) && Tok.isNot(tok::hashhash)) {
+  if (!Tok.isOneOf(tok::hash, tok::hashat, tok::hashhash)) {
 MI->AddTokenToBody(Tok);
 
 // Get the next token of the macro.
@@ -2210,7 +2210,8 @@ void Preprocessor::HandleDefineDirective
   MI->AddTokenToBody(LastTok);
   continue;
 } else {
-  Diag(Tok, diag::err_pp_stringize_not_parameter);
+  Diag(Tok, diag::err_pp_stringize_not_parameter)
+<< LastTok.is(tok::hashat);
 
   // Disable __VA_ARGS__ again.
   Ident__VA_ARGS__->setIsPoisoned(true);

Modified: cfe/trunk/test/Parser/MicrosoftExtensions.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Parser/MicrosoftExtensions.c?rev=265177&r1=265176&r2=265177&view=diff
==
--- cfe/trunk/test/Parser/MicrosoftExtensions.c (original)
+++ cfe/trunk/test/Parser/MicrosoftExtensions.c Fri Apr  1 14:02:20 2016
@@ -35,6 +35,9 @@ void test_ms_alignof_alias(void) {
 /* Charify extension. */
 #define FOO(x) #@x
 char x = FOO(a);
+#define HASHAT #@
+#define MISSING_ARG(x) #@
+/* expected-error@-1 {{'#@' is not followed by a macro parameter}} */
 
 typedef enum E { e1 };
 

Modified: cfe/trunk/test/Preprocessor/stringize_misc.c
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/test/Preprocessor/stringize_misc.c?rev=265177&r1=265176&r2=265177&view=diff
==
--- cfe/trunk/test/Preprocessor/stringize_misc.c (original)
+++ cfe/trunk/test/Preprocessor/stringize_misc.c Fri Apr  1 14:02:20 2016
@@ -1,4 +1,5 @@
-// RUN: %clang_cc1 -E %s | FileCheck -strict-whitespace %s
+#ifdef TEST1
+// RUN: %clang_cc1 -E %s -DTEST1 | FileCheck -strict-whitespace %s
 
 #define M(x, y) #x #y
 
@@ -28,3 +29,13 @@ START_END( {a=1 , b=2;} ) /* braces are
 M(a COMMA b, (a, b)) 
 // CHECK: "a COMMA b" "(a, b)"
 
+#endif
+
+#ifdef TEST2
+// RUN: %clang_cc1 -fsyntax-only -verify %s -DTEST2
+
+#define HASH #
+#define INVALID() #
+// expected-error@-1{{'#' is not followed by a macro parameter}}
+
+#endif


___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D17149: Consolidate and improve the handling of built-in feature-like macros

2016-04-03 Thread Andy Gibbs via cfe-commits 
AndyG updated this revision to Diff 52513.
AndyG added a comment.

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

Thanks.


http://reviews.llvm.org/D17149

Files:
  include/clang/Basic/DiagnosticLexKinds.td
  lib/Lex/PPMacroExpansion.cpp
  test/Preprocessor/feature_tests.c
  test/Preprocessor/invalid-__has_warning1.c
  test/Preprocessor/invalid-__has_warning2.c
  test/Preprocessor/warning_tests.c

Index: test/Preprocessor/warning_tests.c
===
--- test/Preprocessor/warning_tests.c
+++ test/Preprocessor/warning_tests.c
@@ -12,7 +12,7 @@
 #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
 
@@ -22,8 +22,7 @@
 #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
 
@@ -33,7 +32,7 @@
 
 #define MY_ALIAS "-Wparentheses"
 
-// expected-error@+1 2{{expected}}
+// expected-error@+1 {{expected}}
 #if __has_warning(MY_ALIAS)
 #error Alias expansion not allowed
 #endif
Index: test/Preprocessor/invalid-__has_warning2.c
===
--- test/Preprocessor/invalid-__has_warning2.c
+++ test/Preprocessor/invalid-__has_warning2.c
@@ -1,5 +1,5 @@
 // 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();
Index: test/Preprocessor/invalid-__has_warning1.c
===
--- test/Preprocessor/invalid-__has_warning1.c
+++ test/Preprocessor/invalid-__has_warning1.c
@@ -1,5 +1,5 @@
 // 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(
Index: test/Preprocessor/feature_tests.c
===
--- test/Preprocessor/feature_tests.c
+++ test/Preprocessor/feature_tests.c
@@ -55,8 +55,50 @@
 #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
Index: lib/Lex/PPMacroExpansion.cpp
===
--- lib/Lex/PPMacroExpansion.cpp
+++ lib/Lex/PPMacroExpansion.cpp
@@ -1053,9 +1053,8 @@
 
 /// 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)
@@ -1229,8 +1228,8 @@
 /// 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 Prepro

Re: [PATCH] D17149: Consolidate and improve the handling of built-in feature-like macros

2016-04-04 Thread Andy Gibbs via cfe-commits 
AndyG updated this revision to Diff 52535.
AndyG marked 5 inline comments as done.
AndyG added a comment.

Implemented comments.


http://reviews.llvm.org/D17149

Files:
  include/clang/Basic/DiagnosticLexKinds.td
  lib/Lex/PPMacroExpansion.cpp
  test/Preprocessor/feature_tests.c
  test/Preprocessor/invalid-__has_warning1.c
  test/Preprocessor/invalid-__has_warning2.c
  test/Preprocessor/warning_tests.c

Index: test/Preprocessor/warning_tests.c
===
--- test/Preprocessor/warning_tests.c
+++ test/Preprocessor/warning_tests.c
@@ -12,7 +12,7 @@
 #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
 
@@ -22,8 +22,7 @@
 #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
 
@@ -33,7 +32,7 @@
 
 #define MY_ALIAS "-Wparentheses"
 
-// expected-error@+1 2{{expected}}
+// expected-error@+1 {{expected}}
 #if __has_warning(MY_ALIAS)
 #error Alias expansion not allowed
 #endif
Index: test/Preprocessor/invalid-__has_warning2.c
===
--- test/Preprocessor/invalid-__has_warning2.c
+++ test/Preprocessor/invalid-__has_warning2.c
@@ -1,5 +1,5 @@
 // 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();
Index: test/Preprocessor/invalid-__has_warning1.c
===
--- test/Preprocessor/invalid-__has_warning1.c
+++ test/Preprocessor/invalid-__has_warning1.c
@@ -1,5 +1,5 @@
 // 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(
Index: test/Preprocessor/feature_tests.c
===
--- test/Preprocessor/feature_tests.c
+++ test/Preprocessor/feature_tests.c
@@ -55,8 +55,50 @@
 #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 {{nested parentheses not permitted in '__is_identifier'}} 
+#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
Index: lib/Lex/PPMacroExpansion.cpp
===
--- lib/Lex/PPMacroExpansion.cpp
+++ lib/Lex/PPMacroExpansion.cpp
@@ -1053,9 +1053,8 @@
 
 /// 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)
@@ -1229,8 +1228,8 @@
 /// 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 P

Re: [PATCH] D17149: Consolidate and improve the handling of built-in feature-like macros

2016-04-04 Thread Andy Gibbs via cfe-commits 
AndyG added inline comments.


Comment at: lib/Lex/PPMacroExpansion.cpp:1456-1457
@@ +1455,4 @@
+
+// Parse next non-comment, non-annotation token.
+do PP.LexUnexpandedNonComment(Tok); while (Tok.isAnnotation());
+

rsmith wrote:
> 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`.
Ok, annotation tokens are not ignored, but drop down into `Op` and can be 
handled there.


Comment at: lib/Lex/PPMacroExpansion.cpp:1481-1484
@@ +1480,6 @@
+  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();

rsmith wrote:
> 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.
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.


http://reviews.llvm.org/D17149



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

2016-04-05 Thread Andy Gibbs via cfe-commits 
Author: andyg
Date: Tue Apr  5 03:36:47 2016
New Revision: 265381

URL: http://llvm.org/viewvc/llvm-project?rev=265381&view=rev
Log:
Consolidate and improve the handling of built-in feature-like macros

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

Reviewers: doug.gregor, rsmith

Subscribers: rsmith, cfe-commits

Differential Revision: http://reviews.llvm.org/D17149

Modified:
cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td
cfe/trunk/lib/Lex/PPMacroExpansion.cpp
cfe/trunk/test/Preprocessor/feature_tests.c
cfe/trunk/test/Preprocessor/invalid-__has_warning1.c
cfe/trunk/test/Preprocessor/invalid-__has_warning2.c
cfe/trunk/test/Preprocessor/warning_tests.c

Modified: cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td?rev=265381&r1=265380&r2=265381&view=diff
==
--- cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td (original)
+++ cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td Tue Apr  5 03:36:47 2016
@@ -401,6 +401,7 @@ def err_pp_expected_rparen : Error<"expe
 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_nested_paren : Error<"nested parentheses not permitted in %0">;
 def err_pp_colon_without_question : Error<"':' without preceding '?'">;
 def err_pp_division_by_zero : Error<
   "division by zero in preprocessor expression">;

Modified: cfe/trunk/lib/Lex/PPMacroExpansion.cpp
URL: 
http://llvm.org/viewvc/llvm-project/cfe/trunk/lib/Lex/PPMacroExpansion.cpp?rev=265381&r1=265380&r2=265381&view=diff
==
--- cfe/trunk/lib/Lex/PPMacroExpansion.cpp (original)
+++ cfe/trunk/lib/Lex/PPMacroExpansion.cpp Tue Apr  5 03:36:47 2016
@@ -1053,9 +1053,8 @@ static void ComputeDATE_TIME(SourceLocat
 
 /// 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)
@@ -1229,8 +1228,8 @@ static bool HasFeature(const Preprocesso
 /// 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
@@ -1240,7 +1239,6 @@ static bool HasExtension(const Preproces
 return false;
 
   const LangOptions &LangOpts = PP.getLangOpts();
-  StringRef Extension = II->getName();
 
   // Normalize the extension name, __foo__ becomes foo.
   if (Extension.startswith("__") && Extension.endswith("__") &&
@@ -1424,48 +1422,120 @@ static bool EvaluateHasIncludeNext(Token
   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,
+llvm::function_ref<
+  int(Token &Tok,
+  bool &HasLexedNextTok)> Op) {
+  // Parse the initial '('.
+  PP.LexUnexpandedToken(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 ou

Re: [PATCH] D17149: Consolidate and improve the handling of built-in feature-like macros

2016-04-05 Thread Andy Gibbs via cfe-commits 
This revision was automatically updated to reflect the committed changes.
Closed by commit rL265381: Consolidate and improve the handling of built-in 
feature-like macros (authored by AndyG).

Changed prior to commit:
  http://reviews.llvm.org/D17149?vs=52535&id=52664#toc

Repository:
  rL LLVM

http://reviews.llvm.org/D17149

Files:
  cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td
  cfe/trunk/lib/Lex/PPMacroExpansion.cpp
  cfe/trunk/test/Preprocessor/feature_tests.c
  cfe/trunk/test/Preprocessor/invalid-__has_warning1.c
  cfe/trunk/test/Preprocessor/invalid-__has_warning2.c
  cfe/trunk/test/Preprocessor/warning_tests.c

Index: cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td
===
--- cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td
+++ cfe/trunk/include/clang/Basic/DiagnosticLexKinds.td
@@ -401,6 +401,7 @@
 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_nested_paren : Error<"nested parentheses not permitted in %0">;
 def err_pp_colon_without_question : Error<"':' without preceding '?'">;
 def err_pp_division_by_zero : Error<
   "division by zero in preprocessor expression">;
Index: cfe/trunk/test/Preprocessor/invalid-__has_warning2.c
===
--- cfe/trunk/test/Preprocessor/invalid-__has_warning2.c
+++ cfe/trunk/test/Preprocessor/invalid-__has_warning2.c
@@ -1,5 +1,5 @@
 // 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();
Index: cfe/trunk/test/Preprocessor/warning_tests.c
===
--- cfe/trunk/test/Preprocessor/warning_tests.c
+++ cfe/trunk/test/Preprocessor/warning_tests.c
@@ -12,7 +12,7 @@
 #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
 
@@ -22,8 +22,7 @@
 #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
 
@@ -33,7 +32,7 @@
 
 #define MY_ALIAS "-Wparentheses"
 
-// expected-error@+1 2{{expected}}
+// expected-error@+1 {{expected}}
 #if __has_warning(MY_ALIAS)
 #error Alias expansion not allowed
 #endif
Index: cfe/trunk/test/Preprocessor/feature_tests.c
===
--- cfe/trunk/test/Preprocessor/feature_tests.c
+++ cfe/trunk/test/Preprocessor/feature_tests.c
@@ -55,8 +55,50 @@
 #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 '.'}} 
+#if __is_identifier(.abc)// expected-note {{to match this '('}}
+#endif
+
+// expected-error@+1 {{nested parentheses not permitted in '__is_identifier'}} 
+#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
Index: cfe/trunk/test/Preprocessor/invalid-__has_warning1.c
===
--- cfe/trunk/test/Preprocessor/invalid-__has_warning1.c
+++ cfe/trunk/test/Preprocessor/invalid-__has_warning1.c
@@ -1,5 +1,5 @@
 // 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{{untermina

Re: [PATCH] D15636: Reduce false positives in printf/scanf format checker

2016-01-12 Thread Andy Gibbs via cfe-commits 
AndyG added a comment.

Bump! :o)


http://reviews.llvm.org/D15636



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15636: Reduce false positives in printf/scanf format checker

2016-01-25 Thread Andy Gibbs via cfe-commits 
AndyG added a reviewer: rtrieu.
AndyG added a comment.

Richard, you have been recommended to me as a suitable reviewer by David.  If 
that's not ok with you, please could you recommend another!  Thanks, Andy.


http://reviews.llvm.org/D15636



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15636: Reduce false positives in printf/scanf format checker

2016-01-26 Thread Andy Gibbs via cfe-commits 
AndyG added a comment.

Hi Richard,

Thank you for looking at my patch.  I would argue that the code

  printf(minimal ? "%i\n" : "%i: %s\n", code, msg);

is valid and should //not// cause a warning due to unused arguments since at 
least one code path uses all the arguments.  This in my view is the problem 
with the diagnostic as it currently stands.

I should expect that the following would cause the warning:

  printf(minimal ? "%i\n" : "%i: %s\n", code, msg, unused);

which indeed it still does under this patch for the argument 'unused'.  Also, 
where the compiler can determine that a code path is ever followed then it will 
still produce the warning, as in:

  printf(1 ? "%i\n" : "%i: %s\n", code, msg);

Cheers,
Andy


http://reviews.llvm.org/D15636



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Re: [PATCH] D15636: Reduce false positives in printf/scanf format checker

2016-01-28 Thread Andy Gibbs via cfe-commits 
AndyG added a comment.

Yes, but only for the "data argument not used" warning.  All other warnings are 
unaffected by the change, for example:

  test.cpp:9:51: warning: format specifies type 'char *' but the argument has 
type 'int' [-Wformat]  
  printf(minimal ? "%s %s %i\n" : "%i %i %i\n", code, msg);
~~  ^~~~
%d
  test.cpp:9:30: warning: more '%' conversions than data arguments [-Wformat]
  printf(minimal ? "%s %s %i\n" : "%i %i %i\n", code, msg);
  ~^
  test.cpp:9:57: warning: format specifies type 'int' but the argument has type 
'const char *' [-Wformat]
  printf(minimal ? "%s %s %i\n" : "%i %i %i\n", code, msg);
  ~~  ^~~
  %s
  test.cpp:9:45: warning: more '%' conversions than data arguments [-Wformat]
  printf(minimal ? "%s %s %i\n" : "%i %i %i\n", code, msg);
 ~^
  4 warnings generated.

But, as you observed, for the unused argument it only warns for the first 
instance:

  test.cpp:9:53: warning: data argument not used by format string 
[-Wformat-extra-args]
  printf(minimal ? "%i\n" : "%i %s\n", code, msg, msg);
~ ^
  1 warning generated.

This is in comparison to the original diagnostic:

  test.cpp:9:48: warning: data argument not used by format string 
[-Wformat-extra-args]
  printf(minimal ? "%i\n" : "%i %s\n", code, msg, msg);
   ~~^
  test.cpp:9:53: warning: data argument not used by format string 
[-Wformat-extra-args]
  printf(minimal ? "%i\n" : "%i %s\n", code, msg, msg);
~ ^
  2 warnings generated.

However, may I ask what the specific concern is here?  If this diagnostic 
appears then the user knows that **all** the strings do not use this and any 
subsequent argument.  Effectively the original situation doesn't really tell 
the user anything more useful - it only duplicates the warning and actually, as 
can be seen above, it is potentially confusing since it suggests two positions 
for the unused argument(s).  Compare this with the new diagnostic which shows 
only one string, and this string is the first string which uses the most 
arguments, so it is very clear which arguments are not used.

What do you think?  Is this an adequate argument?


http://reviews.llvm.org/D15636



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D15636: Reduce false positives in printf/scanf format checker

2015-12-18 Thread Andy Gibbs via cfe-commits 
AndyG created this revision.
AndyG added a reviewer: cfe-commits.

The printf/scanf format checker is a little over-zealous in handling the 
conditional operator.  This patch reduces work by not checking code-paths that 
are never used and reduces false positives regarding uncovered arguments, for 
example in the code fragment:

printf(minimal ? "%i\n" : "%i: %s\n", code, msg);

http://reviews.llvm.org/D15636

Files:
  include/clang/Sema/Sema.h
  lib/Sema/SemaChecking.cpp
  test/Sema/format-strings.c

Index: test/Sema/format-strings.c
===
--- test/Sema/format-strings.c
+++ test/Sema/format-strings.c
@@ -86,6 +86,19 @@
   printf(i == 0 ? (i == 1 ? "yes" : "no") : "dont know"); // no-warning
   printf(i == 0 ? (i == 1 ? s : "no") : "dont know"); // expected-warning{{format string is not a string literal}}
   printf("yes" ?: "no %d", 1); // expected-warning{{data argument not used by format string}}
+  printf(0 ? "yes %s" : "no %d", 1); // no-warning
+  printf(0 ? "yes %d" : "no %s", 1); // expected-warning{{format specifies type 'char *'}}
+
+  printf(0 ? "yes" : "no %d", 1); // no-warning
+  printf(0 ? "yes %d" : "no", 1); // expected-warning{{data argument not used by format string}}
+  printf(1 ? "yes" : "no %d", 1); // expected-warning{{data argument not used by format string}}
+  printf(1 ? "yes %d" : "no", 1); // no-warning
+  printf(i ? "yes" : "no %d", 1); // no-warning
+  printf(i ? "yes %s" : "no %d", 1); // expected-warning{{format specifies type 'char *'}}
+  printf(i ? "yes" : "no %d", 1, 2); // expected-warning{{data argument not used by format string}}
+
+  printf(i ? "%*s" : "-", i, s); // no-warning
+  printf(i ? "yes" : 0 ? "no %*d" : "dont know %d", 1, 2); // expected-warning{{data argument not used by format string}}
 }
 
 void check_writeback_specifier()
@@ -519,7 +532,7 @@
 
   // Make sure that the "format string is defined here" note is not emitted
   // when the original string is within the argument expression.
-  printf(1 ? "yes %d" : "no %d"); // expected-warning 2{{more '%' conversions than data arguments}}
+  printf(1 ? "yes %d" : "no %d"); // expected-warning{{more '%' conversions than data arguments}}
 
   const char kFormat17[] = "%hu"; // expected-note{{format string is defined here}}}
   printf(kFormat17, (int[]){0}); // expected-warning{{format specifies type 'unsigned short' but the argument}}
Index: lib/Sema/SemaChecking.cpp
===
--- lib/Sema/SemaChecking.cpp
+++ lib/Sema/SemaChecking.cpp
@@ -3004,6 +3004,15 @@
 }
 
 namespace {
+struct UncoveredArgHandler {
+  enum { Unknown = -1, AllCovered = -2 };
+  signed FirstUncoveredArg;
+  const Expr *DiagnosticExpr;
+
+  UncoveredArgHandler() : FirstUncoveredArg(Unknown), DiagnosticExpr(0) { }
+  void Diagnose(Sema &S, bool IsFunctionCall, const Expr *ArgExpr);
+};
+
 enum StringLiteralCheckType {
   SLCT_NotALiteral,
   SLCT_UncheckedLiteral,
@@ -3020,7 +3029,8 @@
   bool HasVAListArg, unsigned format_idx,
   unsigned firstDataArg, Sema::FormatStringType Type,
   Sema::VariadicCallType CallType, bool InFunctionCall,
-  llvm::SmallBitVector &CheckedVarArgs) {
+  llvm::SmallBitVector &CheckedVarArgs,
+  UncoveredArgHandler &UncoveredArg) {
  tryAgain:
   if (E->isTypeDependent() || E->isValueDependent())
 return SLCT_NotALiteral;
@@ -3041,17 +3051,39 @@
 // completely checked only if both sub-expressions were checked.
 const AbstractConditionalOperator *C =
 cast(E);
-StringLiteralCheckType Left =
-checkFormatStringExpr(S, C->getTrueExpr(), Args,
-  HasVAListArg, format_idx, firstDataArg,
-  Type, CallType, InFunctionCall, CheckedVarArgs);
-if (Left == SLCT_NotALiteral)
-  return SLCT_NotALiteral;
+
+// Determine whether it is necessary to check both sub-expressions, for
+// example, because the condition expression is a constant that can be
+// evaluated at compile time.
+bool CheckLeft = true, CheckRight = true;
+
+bool Cond;
+if (C->getCond()->EvaluateAsBooleanCondition(Cond, S.getASTContext())) {
+  if (Cond)
+CheckRight = false;
+  else
+CheckLeft = false;
+}
+
+StringLiteralCheckType Left;
+if (!CheckLeft)
+  Left = SLCT_UncheckedLiteral;
+else {
+  Left = checkFormatStringExpr(S, C->getTrueExpr(), Args,
+   HasVAListArg, format_idx, firstDataArg,
+   Type, CallType, InFunctionCall,
+   CheckedVarArgs, UncoveredArg);
+  if (Left == SLCT_NotALiteral || !CheckRight)
+return Left;
+}
+
 StringLiteralCheckType Right =
 checkFormatStringExpr(S, C->getFalseExpr(), Args,

Re: [PATCH] D15636: Reduce false positives in printf/scanf format checker

2015-12-21 Thread Andy Gibbs via cfe-commits 
AndyG added a comment.

Richard, David,

Sorry, I've added you as reviewers by proximity to recent relevant changes in 
SemaChecking.cpp.  Hope you don't mind.  Please tell me who to redirect to, if 
there are more applicable reviewers.

Cheers
Andy


http://reviews.llvm.org/D15636



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits