HazardyKnusperkeks added a comment. In D93938#2613096 <https://reviews.llvm.org/D93938#2613096>, @atirit wrote:
> In D93938#2612952 <https://reviews.llvm.org/D93938#2612952>, > @HazardyKnusperkeks wrote: > >> In my opinion you should then, either temporarily deactivate the test, or >> fix the bug first. A failing test blocks the pipeline and confuses everyone >> working on the project. > > > > In D93938#2613077 <https://reviews.llvm.org/D93938#2613077>, @curdeius wrote: > >> I got confused about this. I know that there was some discussion about this >> failing test but I thought that the plan was to fix it (as it should). Also, >> that's what one expects in a revision called "Fixed AfterEnum handling" :). > > > > In D93938#2613079 <https://reviews.llvm.org/D93938#2613079>, @MyDeveloperDay > wrote: > >> +1 we are not going to land this with a failing or removed test > > There are two bugs being discussed here. The first is the one fixed here, > where `AfterEnum` is treated as always `true`. The second I found while > adding unit tests for the first bug. I was advised that I shouldn't fix two > bugs with a single differential, especially since the second bug has more to > do with `AllowShortEnumsOnASingleLine` and how it is overridden by > `AfterEnum`. (I think a more appropriate title for a diff handling the second > bug would be "Fixed AllowShortEnumsOnASingleLine handling".) However, if it > is acceptable to do so, then I'll add commits to fix the second bug as well > before asking for this to be merged. > > I agree with @HazardyKnusperkeks that failing tests should not be merged, and > I understand that this patch is not ready yet. > > Please let me know what you think would be the best course of action here. If the bugs are (very) similar, I could live with one fix for both. Otherwise you should fix the other bug first, if its blocking this change. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D93938/new/ https://reviews.llvm.org/D93938 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
