Page MenuHomePhabricator

atirit (Ally Tiritoglu)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 30 2020, 12:49 AM (9 w, 4 d)

Recent Activity

Jan 3 2021

atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

The test still is there; Git is just showing the diff strangely. I didn't modify that test at all. The corner case bug doesn't affect FormatTest.ShortEnums because that test effectively has AfterEnum: false. Should I add cases for AfterEnum: true in that test too? I had figured the new test took care of that.

Jan 3 2021, 1:31 PM · Restricted Project, Restricted Project
atirit updated the diff for D93938: [clang-format] Fixed AfterEnum handling.

Squashed commits

Jan 3 2021, 1:02 PM · Restricted Project, Restricted Project
atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

I don't understand; should every commit I've made be squashed into one and then submitted here?

Jan 3 2021, 12:47 PM · Restricted Project, Restricted Project
atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

AfterEnum: true currently overrides AllowShortEnumsOnASingleLine: true. If this is epxected behaviour then I'll modify the test to accomodate that, but otherwise, there's a separate issue.

Jan 3 2021, 12:24 PM · Restricted Project, Restricted Project
atirit updated the diff for D93938: [clang-format] Fixed AfterEnum handling.

Removed multiple enum names from new test

Jan 3 2021, 12:22 PM · Restricted Project, Restricted Project
atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

The first test fails due to the aforementioned corner case.

Jan 3 2021, 11:53 AM · Restricted Project, Restricted Project
atirit updated the diff for D93938: [clang-format] Fixed AfterEnum handling.

Added unit test

Jan 3 2021, 11:42 AM · Restricted Project, Restricted Project
atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

You need to have these conversations by adding new unit tests that prove your point, I highly doubt I'll personally be willing to accept any revision without more unit tests than this one line change

Jan 3 2021, 10:19 AM · Restricted Project, Restricted Project
atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

I think accepting a revision that includes only a fix for AfterEnum being ignored (not the corner case) and the new unit test would be the best way to go about this, since they're separate bugs. Then I can fix the corner case (and compatibility with the new unit test) in a separate differential.

Jan 3 2021, 2:42 AM · Restricted Project, Restricted Project
atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

OK, I'm getting a few more failed unit tests and I'm really not sure what correct formatting behaviour is anymore, so I'll just ask.

Jan 3 2021, 12:55 AM · Restricted Project, Restricted Project

Jan 2 2021

atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

Turns out the true/true bug goes quite deep. I've managed to resolve the first bit of it with a hack that I'm sure will warrant some criticism, but I haven't familiarised myself with this codebase enough to write a cleaner version.

Jan 2 2021, 8:56 PM · Restricted Project, Restricted Project
atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

After writing a unit test, I've found that a combination of AfterEnum: true and AllowShortEnumsOnASingleLine: true doesn't function properly even in release.. My next revision will include a fix for that alongside the unit test.

Jan 2 2021, 5:40 PM · Restricted Project, Restricted Project
atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

This seems to be that in "Attach" mode, then AllowShortEnumsOnASingleLine=false doesn't attach the brace.

Jan 2 2021, 1:51 PM · Restricted Project, Restricted Project

Jan 1 2021

atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

@MyDeveloperDay Would you mind explaining what changes you're wanting?

Jan 1 2021, 1:57 PM · Restricted Project, Restricted Project

Dec 31 2020

atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

Additionally, this bug has already been reported: https://bugs.llvm.org/show_bug.cgi?id=46927

Dec 31 2020, 3:01 PM · Restricted Project, Restricted Project
atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

@MyDeveloperDay I expect to see exactly that. clang-format currently does not respect AfterEnum, treating it as always true. This is why I changed UnwrappedLineParser.cpp.

Dec 31 2020, 2:09 PM · Restricted Project, Restricted Project

Dec 30 2020

atirit updated the diff for D93938: [clang-format] Fixed AfterEnum handling.

Changed commit username and email

Dec 30 2020, 7:11 PM · Restricted Project, Restricted Project
atirit updated the diff for D93938: [clang-format] Fixed AfterEnum handling.

Fixed the FormatTest.ShortEnums unit test and fixed compatibility with FormatTestCSharp.CSharpKeyWordEscaping and FormatTestJS.EnumDeclarations

Dec 30 2020, 2:35 PM · Restricted Project, Restricted Project
atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

The FormatTestJS.EnumDeclarations test in fact isn't broken, but FormatTest.ShortEnums and FormatTestCSharp.CSharpKeyWordEscaping are.

Dec 30 2020, 2:06 PM · Restricted Project, Restricted Project
atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

From what I can tell the unit tests are broken. Take FormatTest.ShortEnums for example. It passes the following code:

Dec 30 2020, 1:43 PM · Restricted Project, Restricted Project
atirit updated the summary of D93938: [clang-format] Fixed AfterEnum handling.
Dec 30 2020, 1:03 AM · Restricted Project, Restricted Project
atirit requested review of D93938: [clang-format] Fixed AfterEnum handling.
Dec 30 2020, 12:59 AM · Restricted Project, Restricted Project