[PATCH] D23820: Do not warn about format strings that are indexed string literals.

[Richard Smith via cfe-commits]

rsmith added inline comments.

================
Comment at: lib/Sema/SemaChecking.cpp:3842-3843
@@ -3841,2 +3841,4 @@
 
-static void CheckFormatString(Sema &S, const StringLiteral *FExpr,
+static void reckonUpOffset(llvm::APSInt &Offset, llvm::APSInt Addend,
+                           BinaryOperatorKind BinOpKind, bool AddendIsRight) {
+  unsigned BitWidth = Offset.getBitWidth();
----------------
I can't tell from this declaration what this function is for -- what does "reckon up" mean?

================
Comment at: lib/Sema/SemaChecking.cpp:3880
@@ +3879,3 @@
+// This is a wrapper class around StringLiteral to support offsetted string
+// literals as format strings. It takes the offset into account when retruning
+// the string and its length or the source locations to display notes correctly.
----------------
Typo "retruning"

================
Comment at: lib/Sema/SemaChecking.cpp:3891-3892
@@ +3890,4 @@
+  StringRef getString() const {
+    return StringRef(FExpr->getString().data() + Offset,
+                     FExpr->getLength() - Offset);
+  }
----------------
I think you can simplify this to `return FExpr->getString().drop_front(Offset);`

================
Comment at: lib/Sema/SemaChecking.cpp:4143
@@ -4049,2 +4142,3 @@
     if (StrE) {
-      CheckFormatString(S, StrE, E, Args, HasVAListArg, format_idx,
+      if (Offset.isNegative()) {
+        // TODO: It would be better to have an explicit warning for out of
----------------
You should presumably also do this if `Offset` is >= the length of the string literal (we want `printf` and friends to at least find the trailing nul byte).

================
Comment at: lib/Sema/SemaChecking.cpp:4148-4149
@@ +4147,4 @@
+      }
+      FormatStringLiteral *FStr =
+          new FormatStringLiteral(StrE, Offset.sextOrTrunc(64).getSExtValue());
+      CheckFormatString(S, FStr, E, Args, HasVAListArg, format_idx,
----------------
Does this need to be heap-allocated?

================
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 wrote:
> I hope this additional check fixes this issue. As far as I read the code there were none such asserts in isIntegerConstantExpr(). Thanks for explaining this!
OK, I've now checked and the value-dependent case is handled up on line 3953. So just calling `EvaluateAsInt` here is fine after all.


https://reviews.llvm.org/D23820