thanks, looks good!

In D99165#2644243, @sammccall wrote:

The fix doesn't look obviously correct: the side effect of marking the destructor reference seems important if we actually generate code. It's not obvious to me why the type can only be incomplete if there are errors.

This was introduced between clang 8 and clang 9, I would guess by f8ccf052935adaf405e581fd31e8bc634cc5bbc7.
@erik.pilkington
Looking at that patch, mostly this function was just a rename, but there's a new callsite for array-types that seems to be what we're hitting here.
Maybe a slightly less-invasive version would be to guard that callsite with "and if the element type is complete"?

• jrefine the fix of the crash: if the element type of an variable-length array

is incomplete, the array type is incomplete;

• simplify the testcase

Thanks!

In D93300#2621881, @hokein wrote:

hi @stefanp, this patch caused an asan error when running on ninja check-llvm-codegen-powerpc.

llvm-project/llvm/lib/Target/PowerPC/PPCISelDAGToDAG.cpp:1046:48: runtime error: shift exponent 64 is too large for 64-bit type 'uint64_t' (aka 'unsigned long')

hi @stefanp, this patch caused an asan error when running on ninja check-llvm-codegen-powerpc.

This looks like a reasonable solution to me -- if we don't know the function call result type (e.g. dependent-type callExpr in template context), modeling it as a dependent type.

The code looks good, but I don't have a very strong opinion of making the resource-dir as a config option. Do we have a motivation (e.g. user request?) now?

Thanks!

In D97577#2592827, @flx wrote:

In D97577#2592093, @lebedev.ri wrote:

It is best not to change existing tests, but add new ones.

Could elaborate on this, Roman?

In this case the tests use special auto convertible types with the intention to test that the check doesn't trigger because the loop variable needs to be constructed. But the reason they don't trigger is that the loop variable erroneously is already a reference type.

If we keep the tests as is we should rename the test functions and use different types or document that the real reason they don't trigger is that they already are const reference types, but we already have a test for that as well.

thanks, this looks great.

address review comments

Fix and simplify the test.

Thanks, the fix looks good.

address review comment.

fix failure tests.

In D96495#2556897, @njames93 wrote:

Those test failures look related.

address comment, add more tests.

maybe add some data (improvement DecisionForest vs heuristic) in the patch description.

thanks! this looks like a nice improvement!

Thanks for the quick fix.

Thanks!

Looks good, thanks!

thanks, looks better now, just some nits to improve the code readability.

Thanks!

Thanks, this looks right to me.

Thanks, this looks great.

address comments.

Thanks for fixing this!

address comment.

I assume you'll handle the internal integration part ;)

In rGd972d4c749048531953a16b815e07c67e8455a3b#972834, @gmtra wrote:

Hello,

The test case Clang/CodeGenCXX/ms-lookup-template-base-classes.cpp is missing a REQUIRES statement and is failing on Linux validations downstream for us. It needs a:

// REQUIRES: x86-registered-target

Thoughts?

Thank you,
Gaurav

address comments.

Thanks.