It looks like we might have another similar bug: https://github.com/llvm/llvm-project/issues/61589

As I noted in the bug report not doing D.setInvalidType(); does fix this bug and seems harmless since the error diagnostic should prevent us from getting to codegen but it is not clear to me if this has other negative impacts. Reading your replies it is not obvious you looked at this path or not.

Adding clang-language-wg for more visibility

In D146426#4206525, @ilya-biryukov wrote:

In D146426#4206519, @shafik wrote:

Please edit the summary to indicate this fixes: https://github.com/llvm/llvm-project/issues/61441
I am happy someone had more time to dig into this problem after my initial investigation.

Thanks, done!
I wasn't aware of the upstream bug and your prior investigation. It would have definitely saved some time.

Please edit the summary to indicate this fixes: https://github.com/llvm/llvm-project/issues/61441

Thank you for submitting this fix.

• Switched to LexUnexpandedNonComment in more places
• Extended test suite to cover the new cases uncovered in the comments

I would have loved to test the case from https://github.com/llvm/llvm-project/issues/61335 directly but I think in order to do it nicely I need __builtin_memset to be usable in a constant expression context. I will add this to my todo list. I am open to other alternatives for testing this.

• Added release note
• Update CXX status

• Updating diagnostic wording
• Adding Reference to changes from p2448r2 to the comments
• Modified p3 tests to have an extension pass as well

Thank you for the fix.

Can we also add the test from the bug report as a regression test, it looks like the existing test are basically covering the same thing but I have seen stranger bugs so it would be good to explicitly cover it.

Can you add a summary, I realize folks can goto the release notes edit and see the bug report and then go look it up but at least a cursory summary is a little more reviewer friendly.

Is there a way to test this?

LGTM

• Switched to using auto in two if statements
• Added Release notes

Apologies for late review.

LGTM, I wlll let @royjacobson give the final approval after reviewing codegen tests.

In D145376#4176228, @EricWF wrote:

This change is not correct and cannot proceed.

It changes the meaning of this code:

template <class T, class U, class = U[std::declval<T>()]>
constexpr bool xxx(int) { return true; }

template <class T, class U>
constexpr bool xxx(long) { return false; }

constexpr bool y = xxx<long long, int>(0);

declval has been constructed very carefully to performant, and correct in all the weird cases where Clang will try to eagerly instantiate something (Ex. when it tries to check for C++11 narrowing when evaluating constant expressions).
Please be cautious when modifying weird things like declval.

This LGTM, I agree w/ @cor3ntin about quoting [expr.cont] to document this choice.

Besides the nitpick, LGTM

Thank you for fixing this so quickly.

I think these two bug reports: https://github.com/llvm/llvm-project/issues/46132 and https://github.com/llvm/llvm-project/issues/61118 may be related to this change.

LGTM

LGTM

LGTM, the modules failures is a previous known issue.

LGTM

Also please add a release note before landing the fix, thank you.

Thank you for this patch, it looks good.

Thank you for taking the time to submit a patch.

• Add release note

I think VisitCastExpr is the right place to start looking for what to do.

LGTM

So we are only checking global constructors b/c it is valid in a constant expression context to initialize a record and not initialize all their fields as long as we don't use any of those fields.

If I look at the clang docs for Wconversion I see it includes -Wshorten-64-to-32 which I believe this is a case of. I think maybe the warning needs a better warning for this case?

Ping

LGTM

In D143891#4122731, @aaron.ballman wrote:

In D143891#4122668, @royjacobson wrote:

In D143891#4122660, @aaron.ballman wrote:

This is an ABI breaking change, isn't it? (The type trait now returns something different than it did before, which could change instantiations or object layout.)

Technically it is, but it only affects code that relies on constrained default constructors, which we're only going to support in Clang 16. So if we backport this to 16 it's not very problematic.

Hmmm, if it's true that this only changes the behavior of a type with a constrained default constructor, then I think it's fine despite being an ABI break (we never claimed full support for concepts, so anyone relying on a particular behavior was mistaken to do that). I can't quite convince myself this doesn't impact other cases though -- the logic for computing triviality is nontrivial itself (pun intended), so I'm really only concerned that __is_trivial and friends return a different value for a non-constrained type. However, I also can't come up with a test case whose behavior changes either.

In D142867#4108079, @eaeltsin wrote:

The warning now fires even if overflow is prevented with if constexpr:

if constexpr (width <= 64) {
  if constexpr (width == 64) {
    return 1;
  }
  return -static_cast<int64_t>(uint64_t{1} << (width - 1));
}

https://godbolt.org/z/M3xdcKd3M

WG21 is meeting all this week, so a bunch of folks who should take a look at this may not get around to it right away.

It looks like https://github.com/llvm/llvm-project/issues/60543 is hitting your added llvm_unreachable("missing serialization code for annotation token"); is this expected.

• Add release note.

The build failures look unrelated to this change.

• Fix up release notes

• Add release note

Adding clang-language-wg for more visibility.