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

[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