shafik added a comment.

In D149389#4306605 <https://reviews.llvm.org/D149389#4306605>, @ayzhao wrote:

> In D149389#4303966 <https://reviews.llvm.org/D149389#4303966>, @shafik wrote:
>
>> So the change makes sense but the original issue: 
>> https://github.com/llvm/llvm-project/issues/62266 it worked for the first 
>> once but not the second, it also worked for the non-paren aggregate init. So 
>> I feel like the explanation is missing some details.
>
> Patch description has been updated with an explanation.
>
>> It seems like the codegen test should be mimicing the test from the bug 
>> report more closely w/ two paren inits.
>
> The bug was that Clang wasn't calling the default-member intializer in the 
> first initializer, which the CodeGen test asserts. The second initializer is 
> irrelevant - it's only function is to provide the expected value of the 
> initialized field. In fact, changing the test assertion to be the same as 
> that in the bug report (asserting that the values of the fields in the two 
> objects are equal) is worse because that test would pass if we 
> value-initialize the fields instead of calling the default initialization 
> expression for both objects.

Apologies, I was not alluding to the comparing of the results of the two 
initializations but ensuring that it does the right thing for not just the 
first but also the second. So we did not change the bug in some strange way 
where the first now succeeds but now the second fails.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D149389/new/

https://reviews.llvm.org/D149389

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to