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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
