[PATCH] D93938: [clang-format] Fixed AfterEnum handling

[Ally Tiritoglu via Phabricator via cfe-commits]

atirit added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D93938