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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
