[PATCH] D119136: [clang] Implement Change scope of lambda trailing-return-type

Wed Apr 20 11:31:39 PDT 2022

aaron.ballman added a comment.

In D119136#3462609 <https://reviews.llvm.org/D119136#3462609>, @MaskRay wrote:

> In D119136#3462579 <https://reviews.llvm.org/D119136#3462579>, @aaron.ballman wrote:
>
>> In D119136#3462570 <https://reviews.llvm.org/D119136#3462570>, @MaskRay wrote:
>>
>>> Sorry but I've reverted this patch and all its fixups in c79e6007edef4b0044be93c4ffff64dc806ac687 <https://reviews.llvm.org/rGc79e6007edef4b0044be93c4ffff64dc806ac687> and 0f5dbfd29ae0df215a01aff80d29255bb799fed0 <https://reviews.llvm.org/rG0f5dbfd29ae0df215a01aff80d29255bb799fed0> .
>>> See https://reviews.llvm.org/D123909#3461716 for another case not considered.
>>
>> Please double check with the patch author and reviewers before unilaterally reverting multiple commits with no notice and no failing build bots. I do not see a valid justification for these reverts -- the concerns have been true positives so far (or have generated core issues that WG21 is still discussing), and @cor3ntin has been highly responsive with addressing the fallout. This is a significant amount of churn that I don't think should have happened.
>
> Reply to both this message and https://reviews.llvm.org/D123909#3462560:
>
> I am sorry but this thread has gained reports from multiple independent parties about breakage, and I just thought the followups did not fix all issues.

AFAIK, all issues have either been addressed, are true positives, or are a matter for WG21 to figure out what to do with. However, I'm not certain it was clear from the reviews that this was the case, so it's understandable there's some amount of "did we get everything?" confusion.

> I confess I do not really understand the subtle areas in the language.
> My decision to revert was mostly driven by fixups' descriptions like "but did not properly handled dependant context,"
>
> - which made me believe there were indeed bugs in the implementation and new corner cases just emerged.
>
> I'd agree that there is invalid code which is not correctly rejected, but the fixups mixed with bugfixes made the whole picture unclear.

The trouble here was: WG21 adopted a proposal intended as a bugfix, but it was never implemented before (to my knowledge). We went and did the implementation and discovered that the bugfix paper actually introduces different bugs (hence us breaking libstdc++; the diagnostic was valid, but the paper didn't likely intend to make that code invalid). CWG2569 was filed to try to address this, but the issue hasn't been resolved by WG21 yet. So what @cor3ntin has done is to address *only* the libstdc++ failure case (to keep important system headers working) until WG21 figures out whether we need to be relaxed in more cases.

This does mean that there may be some code broken that WG21 will later decide should not be broken, but that's a matter for the Core Issue; these changes are to implement P2036R3 which is a C++23 feature.

> I apology if I did make wrong reverts.
> At this point, the reverts have been made. I hope that starting from a clean state is not too bad. I am grateful to @cor3ntin who is highly responsive with addressing the fallout.

Thanks for the apology, but I still think the reverts were premature, so hopefully we don't do this again. The issue with reverting all of these is that you put the burden back onto a relatively new contributor to try to figure out how to reapply all these changes when there wasn't actually a need to do so.

In terms of where we go from here, there's a few options:

1. revert the reverts; this is easiest, but causes the most churn
2. merge all three patches into one (squashed) patch and commit that; this requires a little bit more work, but causes less churn
3. assume that P2036R3 is not implementable until WG21 resolves CWG2569 because it breaks too much code

I think I have a preference for #2 as that will hopefully leave us with the most clear git history for when we do a blame later, but I could live with #1. If we find that there is significant broken code still, but it's not in system headers, then I think we should explore #3, but I don't think we have evidence that we're in this situation either.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119136