hubert.reinterpretcast marked an inline comment as done.
hubert.reinterpretcast 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 =
----------------
rsmith wrote:
> Why do we need to do this explicitly, rather than allowing to simply happen
> as part of the first `handleLValueToRValueConversion` below?
We want the error message to be produced even if we bail early on not having a
valid designator.
================
Comment at: lib/AST/ExprConstant.cpp:6150
+ QualType CharTy =
+ Info.Ctx.getBaseElementType(getType(Result.getLValueBase()));
+ const bool IsRawByte = BuiltinOp == Builtin::BImemchr ||
----------------
rsmith wrote:
> 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.
Got it; I also updated the tests to be sensitive to this issue.
================
Comment at: test/CodeGenCXX/builtins.cpp:41
+ matchedFirstByteIn04030201();
+ // CHECK: call void
@_ZN27MemchrMultibyteElementTests26matchedFirstByteIn04030201Ev()
+ }
----------------
rsmith wrote:
> hubert.reinterpretcast wrote:
> > 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.
> Seems fine to have a test that we don't fold certain cases at all, with a
> comment saying that it's OK if we start folding them to a constant and what
> that constant should be.
>
> Generally the principle is that we want our unit tests to test as few layers
> of the stack as possible; the chosen test directory should ideally correspond
> to the layer under test. If you'd really like this to be more of an
> integration test, that's fine too (eg, if you have code like this in a
> platform header that really must be handled a certain way), but then it would
> belong in `test/Integration`. If you do put it in `test/Integration`, it'd
> then be reasonable to include -O in the test flags too, if that makes sense
> for what you want to test.
I think a `SemaCXX` test based on the following pattern would work:
```
constexpr decltype(sizeof 0) Good = 42, Bad = 43;
template <decltype(sizeof 0)> struct A;
struct NotBad {};
template <> struct A<Good> : NotBad {};
template <typename T, decltype(sizeof 0) N> A<N> *evalFor(T (&)[N]);
NotBad *evalFor(...);
void chk(NotBad *);
void f() {
int x[__builtin_memcmp("", "", 1) == 0 ? Good : Bad];
chk(evalFor(x));
}
```
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
