This revision was automatically updated to reflect the committed changes.
Closed by commit rL281527: Do not warn about format strings that are indexed
string literals. (authored by srhines).
Changed prior to commit:
https://reviews.llvm.org/D23820?vs=71286=71416#toc
Repository:
rL LLVM
srhines added a comment.
I will take care of submitting this. Thanks for the reviews everybody!
https://reviews.llvm.org/D23820
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
meikeb marked an inline comment as done.
meikeb added a comment.
Thanks for reviewing my patch! It would be great if someone could submit this
patch for me.
https://reviews.llvm.org/D23820
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
meikeb marked an inline comment as done.
Comment at: lib/Sema/SemaChecking.cpp:3864-3867
@@ +3863,6 @@
+ResOffset = Offset.sadd_ov(Addend, Ov);
+ else {
+assert(AddendIsRight && BinOpKind == BO_Sub &&
+ "operator must be add or sub with addend on the right");
+
meikeb marked 2 inline comments as done.
meikeb added a comment.
https://reviews.llvm.org/D23820
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
meikeb updated this revision to Diff 71284.
meikeb added a comment.
Try to improve offset sum helper function name and fix style issues.
https://reviews.llvm.org/D23820
Files:
lib/Sema/SemaChecking.cpp
test/Sema/format-strings.c
Index: test/Sema/format-strings.c
rsmith added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:3864-3867
@@ +3863,6 @@
+ResOffset = Offset.sadd_ov(Addend, Ov);
+ else if (AddendIsRight && BinOpKind == BO_Sub)
+ResOffset = Offset.ssub_ov(Addend, Ov);
+ else
+assert(AddendIsRight && BinOpKind
rsmith accepted this revision.
rsmith added a comment.
This revision is now accepted and ready to land.
This basically looks fine to me now. I'm not 100% sold on
`sumUpStringLiteralOffset` being the best name, but I think we have better
things to worry about, and it's good enough. Just a couple
meikeb added a comment.
I explained why I chose the names that you commented on. Feel free to add your
thoughts if you still think another name would be more fitting.
Comment at: lib/Sema/SemaChecking.cpp:3842
@@ -3841,2 +3841,3 @@
-static void CheckFormatString(Sema , const
meikeb updated this revision to Diff 71199.
meikeb marked 7 inline comments as done.
meikeb added a comment.
Fix typos and add assert to sum up offset helper.
https://reviews.llvm.org/D23820
Files:
lib/Sema/SemaChecking.cpp
test/Sema/format-strings.c
Index: test/Sema/format-strings.c
srhines added a comment.
My comment is mostly naming considerations to improve clarity. I do have
concerns though about the unhandled else path.
Comment at: lib/Sema/SemaChecking.cpp:3842
@@ -3841,2 +3841,3 @@
-static void CheckFormatString(Sema , const StringLiteral *FExpr,
meikeb marked 6 inline comments as done.
meikeb added a comment.
Thanks for taking the time and doing these great reviews! Really appreciated!
Comment at: lib/Sema/SemaChecking.cpp:4143-4150
@@ -4049,3 +4142,10 @@
if (StrE) {
- CheckFormatString(S, StrE, E, Args,
rsmith added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:3842-3843
@@ -3841,2 +3841,4 @@
-static void CheckFormatString(Sema , const StringLiteral *FExpr,
+static void reckonUpOffset(llvm::APSInt , llvm::APSInt Addend,
+ BinaryOperatorKind
meikeb marked 4 inline comments as done.
Comment at: lib/Sema/SemaChecking.cpp:4166-4167
@@ +4165,4 @@
+if (BinOp->isAdditiveOp()) {
+ bool LIsInt = false;
+ bool RIsInt = false;
+ // Prevent asserts triggering in EvaluateAsInt by checking if we deal
with
meikeb updated this revision to Diff 71043.
meikeb added a comment.
Fix the mentioned issues.
https://reviews.llvm.org/D23820
Files:
lib/Sema/SemaChecking.cpp
test/Sema/format-strings.c
Index: test/Sema/format-strings.c
===
rsmith added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:4089-4090
@@ +4088,4 @@
+if (BinOp->isAdditiveOp()) {
+ bool LIsInt = BinOp->getLHS()->EvaluateAsInt(LResult, S.Context);
+ bool RIsInt = BinOp->getRHS()->EvaluateAsInt(RResult, S.Context);
+
meikeb added inline comments.
Comment at: lib/Sema/SemaChecking.cpp:4089-4090
@@ +4088,4 @@
+if (BinOp->isAdditiveOp()) {
+ bool LIsInt = BinOp->getLHS()->EvaluateAsInt(LResult, S.Context);
+ bool RIsInt = BinOp->getRHS()->EvaluateAsInt(RResult, S.Context);
+
rsmith added a comment.
Please also handle expressions of the form `&"str"[i]`. We warn (under
`-Wstring-plus-int`) for `"str" + i` and suggest rewriting into that form, so
we should also handle that form here.
Comment at: lib/Sema/SemaChecking.cpp:3864
@@ -3864,1 +3863,3 @@
meikeb marked an inline comment as done.
Comment at: lib/Sema/SemaChecking.cpp:3856
@@ -3855,3 +3855,3 @@
// format string, we will usually need to emit a warning.
// True string literals are then checked by CheckFormatString.
static StringLiteralCheckType
I'm
19 matches
Mail list logo
