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