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

[Stephen Hines via cfe-commits]

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 &S, const StringLiteral *FExpr,
+static void sumUpStringLiteralOffset(llvm::APSInt &Offset, llvm::APSInt Addend,
+                                     BinaryOperatorKind BinOpKind,
----------------
Is "computeStringLiteralOffset" or "calculate..." a better name here?

================
Comment at: lib/Sema/SemaChecking.cpp:3844
@@ +3843,3 @@
+                                     BinaryOperatorKind BinOpKind,
+                                     bool AddendIsRight) {
+  unsigned BitWidth = Offset.getBitWidth();
----------------
Is "Operand" better than "Addend"? In particular, there is the possibility that we do subtraction of the value instead of addition, so "Addend" makes it a bit confusing. Of course, I then would expect "OperandIsRight" instead of "AddendIsRight" too.

================
Comment at: lib/Sema/SemaChecking.cpp:3852
@@ +3851,3 @@
+  }
+  // Align the bit width of the APSInts.
+  if (AddendBitWidth > BitWidth) {
----------------
Align -> Canonicalize or Adjust

This is confusing here as "Align" already has too many overloaded meanings in programming (that could be relevant to bitwidths). Canonicalize or Adjust don't have this problem.

================
Comment at: lib/Sema/SemaChecking.cpp:3860
@@ +3859,3 @@
+
+  bool Ov = false;
+  llvm::APSInt ResOffset = Offset;
----------------
Ov -> Overflow

================
Comment at: lib/Sema/SemaChecking.cpp:3865
@@ +3864,3 @@
+  else if (AddendIsRight && BinOpKind == BO_Sub)
+    ResOffset = Offset.ssub_ov(Addend, Ov);
+
----------------
What happens if someone passes something that isn't caught by these two cases? Should we be returning an indicator that the calculation failed? If not, should we assert here?

================
Comment at: lib/Sema/SemaChecking.cpp:3867
@@ +3866,3 @@
+
+  // We add a offset to a pointer here so we should support a offset as big as
+  // possible.
----------------
2 places to fix: "a offset" -> "an offset"


https://reviews.llvm.org/D23820