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

[Meike Baumgärtner via cfe-commits]

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 &S, const StringLiteral *FExpr,
+static void sumUpStringLiteralOffset(llvm::APSInt &Offset, llvm::APSInt Addend,
+                                     BinaryOperatorKind BinOpKind,
----------------
srhines wrote:
> Is "computeStringLiteralOffset" or "calculate..." a better name here?
I thought about that but decided to go with sumUp because compute or calculate sounds like this function would do what we actually do what the caller of this function does (computing the offset). This is just a nice helper to sum up the offset we already have with another piece of offset.

================
Comment at: lib/Sema/SemaChecking.cpp:3844
@@ +3843,3 @@
+                                     BinaryOperatorKind BinOpKind,
+                                     bool AddendIsRight) {
+  unsigned BitWidth = Offset.getBitWidth();
----------------
srhines wrote:
> 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.
Clang summarizes sub and add as "additive" operands. This is why I think this is fitting. Operand is misleading because it includes a lot more operands than add and sub imo.

================
Comment at: lib/Sema/SemaChecking.cpp:3860
@@ +3859,3 @@
+
+  bool Ov = false;
+  llvm::APSInt ResOffset = Offset;
----------------
srhines wrote:
> Ov -> Overflow
I named that in compliance with clang naming. E.g. sadd_ov. It is common in this file to abbreviate variable names with 1-3 characters.


https://reviews.llvm.org/D23820