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

[Corentin Jabot via Phabricator via cfe-commits]

cor3ntin added a comment.

Sorry for the delayed reply.

In D119136#3462660 <https://reviews.llvm.org/D119136#3462660>, @aaron.ballman wrote:

> 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.

Yes, as far as I'm aware all the implementation issues with this patch has been addressed (or are undiscovered),  so the remaining issues are that a C++ proposal (approved as a Defect Report in C++11 and up) do break existing code.
The issue has been raised in the C++ committee and clang implements https://github.com/cplusplus/CWG/issues/31 to mitigate the issue.
You will note that the resolution addresses `decltype(id)` but not `decltype(*id)` - there is no reasonable way to make the later work in a coherent fashion.

I think it would be tremendously helpful if you could share more of the reports you had internally, as it might affect how the C++ committee decides to resolves this.

In the meantime, clang could make that pattern well formed in older language modes, it something we have discussed, however I'm not sure you want to rely on code that will be made ill-formed when switching to C++23.
And the fact that `decltype(*id)` would have a different meaning in different part of the lambda expression (`[id] (decltype(*id)) ->decltype(*id)` may be 2 different types) makes me very uncomfortable -  it's the reason the C++ committee decided to make that ill-formed to begin with.

It was the intent of WG21 for this change to be breaking. But as Aaron said, Clang is the first implementation to support it, so we are discovering unforeseen issues with it. 
I think we should quantify the extent of the problem before taking drastic actions, as that will help both WG21 and Clang decide what are the most effective mitigation strategy, if any.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D119136