rtrieu accepted this revision.
rtrieu added a comment.
This revision is now accepted and ready to land.
LGTM
The other issues brought up in this review can go in follow up patches.
http://reviews.llvm.org/D15636
___
cfe-commits mailing list
cfe-comm
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();
+
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
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 s
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
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/
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'
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
rtrieu added a comment.
In http://reviews.llvm.org/D15636#352532, @AndyG wrote:
> Richard, are you happy with this latest version? Can I proceed to commit it?
>
> Thanks.
No, this is not ready yet. This code is old and needs a bit of clean up, and
because your patch touches it, you will be o
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-c
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
rtrieu added a comment.
In http://reviews.llvm.org/D15636#343856, @AndyG wrote:
> 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:
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-a
rtrieu added a comment.
Oops, forget to hit send on my last comment.
My concern is something like:
printf(condition ? "first message: %d" : "second message: %d", 5, 10);
There's nothing in the code implying that either message is the one that should
be using two arguments, so which one will
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
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]
rtrieu added a comment.
It looks like you are limiting to one diagnostic per printf, no matter the
number of format strings. How is the case when multiple format strings would
trigger the warning?
http://reviews.llvm.org/D15636
___
cfe-commits ma
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 th
rtrieu added a comment.
Hi Andy,
Could you give some motivation for this patch? From your example:
printf(minimal ? "%i\n" : "%i: %s\n", code, msg);
I would expect the first format string to produce a warning that msg is not
used.
http://reviews.llvm.org/D15636
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-c
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
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
_
22 matches
Mail list logo
