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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
