rsmith added inline comments.
================
Comment at: lib/AST/ExprConstant.cpp:6147-6148
+ return ZeroInitialization(E);
+ if (!Result.checkNullPointerForFoldAccess(Info, E, AK_Read))
+ return false;
+ QualType CharTy =
----------------
Why do we need to do this explicitly, rather than allowing to simply happen as
part of the first `handleLValueToRValueConversion` below?
================
Comment at: lib/AST/ExprConstant.cpp:6150
+ QualType CharTy =
+ Info.Ctx.getBaseElementType(getType(Result.getLValueBase()));
+ const bool IsRawByte = BuiltinOp == Builtin::BImemchr ||
----------------
Considering the LValue base here is incorrect. The object or array we're
copying could be a subobject, and the complete object type (the type of the
lvalue base) is irrelevant for such copies. Use
`Result.Designator.getType(Info.Ctx)` to find the type that the lvalue
designates. (You can bail out here if the designator is invalid;
`handleLValueToRValueConversion` will always fail on such cases anyway.
================
Comment at: lib/AST/ExprConstant.cpp:6159-6160
+ // Give up on byte-oriented matching against multibyte elements.
+ if (IsRawByte && Info.Ctx.getTypeSize(CharTy) > Info.Ctx.getCharWidth())
+ return false;
// Figure out what value we're actually looking for (after converting to
----------------
Please add an assert here that the types do match in the non-raw-byte case.
(They must, or the subobject designator would be invalid; the only special case
is that a `void*` can point to anything, and only the `memchr` variants here
take a `void*`.)
================
Comment at: lib/AST/ExprConstant.cpp:8433-8436
+ QualType CharTy1 =
+ Info.Ctx.getBaseElementType(getType(String1.getLValueBase()));
+ QualType CharTy2 =
+ Info.Ctx.getBaseElementType(getType(String2.getLValueBase()));
----------------
Per above comment, this is not correct.
================
Comment at: lib/AST/ExprConstant.cpp:8467
+ assert(MaxLength);
+ for (;;) {
+ APValue Char1, Char2;
----------------
Style nit: please use `while (true)` rather than `for (;;)`
================
Comment at: lib/AST/ExprConstant.cpp:8471-8476
+ // We have compatible in-memory widths, but a possible type and
internal
+ // representation mismatch. Assuming two's complement representation,
+ // including 0 for `false` and 1 for `true`, we can check an
appropriate
+ // number of elements for equality even if they are not byte-sized.
+ const APSInt Char1InMem = Char1.getInt().extOrTrunc(CharTy1Width);
+ const APSInt Char2InMem = Char2.getInt().extOrTrunc(CharTy1Width);
----------------
The *only* possible problem here is for `bool`, right? This comment would be
clearer if it said that.
================
Comment at: lib/AST/ExprConstant.cpp:8484
+ }
+ return false;
+ }
----------------
Please add a comment before this return indicating that the result depends on
byte order (and maybe a FIXME to compare the bytes in the right order rather
than bailing out). We're going to need to deal with the byte order /
representation problem for `std::bit_cast` pretty soon anyway.
================
Comment at: lib/AST/ExprConstant.cpp:8488-8489
+ return false;
+ if (MaxLength <= LengthPerElement)
+ break;
+ MaxLength -= LengthPerElement;
----------------
This looks wrong. If `0 < MaxLength && MaxLength < LengthPerElement`, we're
supposed to compare *part of* the next element; this current approach doesn't
do that. (If you did one more full-element compare, that'd be conservatively
correct because we're only checking for equality, not ordering, but perhaps we
should only permit cases where `MaxLength` is a multiple of `LengthPerElement`?
================
Comment at: test/CodeGenCXX/builtins.cpp:41
+ matchedFirstByteIn04030201();
+ // CHECK: call void
@_ZN27MemchrMultibyteElementTests26matchedFirstByteIn04030201Ev()
+ }
----------------
Please test this in a way that doesn't rely on IR generation
constant-evaluating `if` conditions. Moreover, there's nothing about IR
generation that you're actually testing here, so please phrase this as a `Sema`
test instead (eg, check that a VLA bound gets folded to a constant). Likewise
for the other tests below.
Repository:
rC Clang
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D55510/new/
https://reviews.llvm.org/D55510
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
