hubert.reinterpretcast added inline comments.
================
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
----------------
rsmith wrote:
> 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*`.)
Will do.
================
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);
----------------
rsmith wrote:
> The *only* possible problem here is for `bool`, right? This comment would be
> clearer if it said that.
We do assume two's complement representation (which C89 does not require to be
true); so, `bool` is more problematic than other things, but the comment is not
just about `bool`.
================
Comment at: lib/AST/ExprConstant.cpp:8488-8489
+ return false;
+ if (MaxLength <= LengthPerElement)
+ break;
+ MaxLength -= LengthPerElement;
----------------
rsmith wrote:
> 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`?
We //are// doing one more full-element compare to be conservatively correct.
There is a test for that.
================
Comment at: test/CodeGenCXX/builtins.cpp:41
+ matchedFirstByteIn04030201();
+ // CHECK: call void
@_ZN27MemchrMultibyteElementTests26matchedFirstByteIn04030201Ev()
+ }
----------------
rsmith wrote:
> 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.
Not all of these cases can fold successfully with the current implementation.
The check would need to be that we either fold (and get the right value), or
don't fold at all.
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
