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

[Richard Smith via cfe-commits]

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 @@
+                      UncoveredArgHandler &UncoveredArg,
+                      int64_t &Offset) {
  tryAgain:
----------------
Why is this passed by reference? It's just an input, not an output, right?

================
Comment at: lib/Sema/SemaChecking.cpp:4062-4067
@@ +4061,8 @@
+        // into the original string literal.
+        StrE = StringLiteral::Create(S.Context,
+                                     StrE->getString().drop_front(Offset),
+                                     StrE->getKind(),
+                                     StrE->isPascal(),
+                                     StrE->getType(),
+                                     StrE->getLocStart());
+      } else if (Offset < 0) {
----------------
This doesn't seem like it preserves enough information for the downstream code to give correct caret diagnostics pointing at locations within the string. It seems like it would be extremely complicated to maintain the necessary invariants to make that work (you'd need to create a fake string literal source buffer so that the `StringLiteralParser` can reparse it, for whichever of the string literal tokens the offset ends up within). Have you looked at how much work it'd be to feed a starting offset into `CheckFormatString` instead?

================
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);
+
----------------
What happens if one of these expressions is value-dependent? The evaluator can crash or assert if given a value-dependent expression. If we don't defer these checks in dependent contexts, you'll need to handle that possibility somehow.

Example:

  template<int N> void f(const char *p) {
    printf("blah blah %s" + N, p);
  }

================
Comment at: lib/Sema/SemaChecking.cpp:4097-4100
@@ +4096,6 @@
+          if (LIsInt) {
+            Offset += LResult.getExtValue();
+            E = BinOp->getRHS();
+          } else {
+            Offset += RResult.getExtValue();
+            E = BinOp->getLHS();
----------------
This will assert if the result doesn't fit into 64 bits, and it's not guaranteed to (if one of the operands was cast to `__int128`, for instance). You could use `getLimitedValue` instead, with some suitable limit.


https://reviews.llvm.org/D23820