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

[Richard Trieu via cfe-commits]

rtrieu added a comment.

A few more comments.

In http://reviews.llvm.org/D15636#358588, @AndyG wrote:

> Patch additionally re-based off r261522.


It's usually a bad idea to rebase in the middle of a string of patches.  Phabricator isn't aware of revisions, so while a base to latest patch will show up fine, attempting to diff the original patch to the latest will show all the upstream changes as well.


================
Comment at: lib/Sema/SemaChecking.cpp:3279
@@ +3278,3 @@
+  void Update(signed NewFirstUncoveredArg, const Expr *StrExpr) {
+    // Don't update if a previous string covers all arguments.
+    if (FirstUncoveredArg == AllCovered)
----------------
Add an assert that NewFirstUncoveredArg >= 0

================
Comment at: lib/Sema/SemaChecking.cpp:3284-3288
@@ +3283,7 @@
+    if (NewFirstUncoveredArg < 0) {
+      // A string has been found with all arguments covered, so clear out
+      // the diagnostics.
+      FirstUncoveredArg = AllCovered;
+      DiagnosticExprs.clear();
+      return;
+    }
----------------
Move this to a new function "setAllCovered()"

================
Comment at: lib/Sema/SemaChecking.cpp:3823-3833
@@ -3822,13 +3904,1 @@
     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());
-        }
-      }
-    }
----------------
Use two different function calls depending on the result of the find_first().  This also preserves the existing assert.
```
signed notCoveredArg = CoveredArgs.find_first();
if (notCoveredArg >= 0) {
  assert((unsigned)notCoveredArg < NumDataArgs);
  UncoveredArg.Update(notCoveredArg, OrigFormatExpr);
} else {
  UncoveredArg.setAllCovered();
}
```



================
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);
   }
----------------
AndyG wrote:
> Rather than `FExpr`, I think this should be `OrigFormatExpr`?
OrigFormatExpr is probably better.

================
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();
+
----------------
That raises the question why the first element is different from the rest.

================
Comment at: lib/Sema/SemaChecking.cpp:5081
@@ -4981,3 +5080,3 @@
                                             Type == Sema::FST_FreeBSDKPrintf))
       H.DoneProcessing();
   } else if (Type == Sema::FST_Scanf) {
----------------
Not exactly what I said, but this works too.


http://reviews.llvm.org/D15636