Page MenuHomePhabricator

atirit (Ally Tiritoglu)
User

Projects

User does not belong to any projects.

User Details

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

Recent Activity

Mar 17 2021

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

I've found yet another bug. When AllowShortEnumsOnASingleLine is true and a multiline enum is forced (by line length, trailing comma, etc.), multiple names for the enum are placed on separate lines.

Mar 17 2021, 10:38 AM · Restricted Project, Restricted Project
atirit added inline comments to D93938: [clang-format] Fixed AfterEnum handling.
Mar 17 2021, 8:17 AM · Restricted Project, Restricted Project
atirit updated the diff for D93938: [clang-format] Fixed AfterEnum handling.

Removed strlen call and added tests

Mar 17 2021, 12:50 AM · Restricted Project, Restricted Project

Mar 16 2021

atirit added inline comments to D93938: [clang-format] Fixed AfterEnum handling.
Mar 16 2021, 7:24 PM · Restricted Project, Restricted Project

Mar 15 2021

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

Fixed remote build

Mar 15 2021, 7:20 PM · Restricted Project, Restricted Project
atirit updated the diff for D93938: [clang-format] Fixed AfterEnum handling.

Implemented requested changes

Mar 15 2021, 11:13 AM · Restricted Project, Restricted Project
atirit added inline comments to D93938: [clang-format] Fixed AfterEnum handling.
Mar 15 2021, 11:11 AM · Restricted Project, Restricted Project
atirit added inline comments to D93938: [clang-format] Fixed AfterEnum handling.
Mar 15 2021, 8:10 AM · Restricted Project, Restricted Project
atirit updated the diff for D93938: [clang-format] Fixed AfterEnum handling.

Build fix

Mar 15 2021, 12:56 AM · Restricted Project, Restricted Project
atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

I would like to request that this commit be considered for merging.

Mar 15 2021, 12:53 AM · Restricted Project, Restricted Project
atirit updated the diff for D93938: [clang-format] Fixed AfterEnum handling.

Added comments for the previous commit's changes and cleaned up those changes

Mar 15 2021, 12:52 AM · Restricted Project, Restricted Project

Mar 13 2021

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

Fixed AfterEnum's compatibility with AllowShortEnumsOnASingleLine

Mar 13 2021, 11:51 PM · Restricted Project, Restricted Project

Mar 10 2021

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

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.

Mar 10 2021, 1:16 AM · Restricted Project, Restricted Project

Mar 8 2021

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

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.

Mar 8 2021, 11:40 PM · Restricted Project, Restricted Project
atirit added a comment to D93938: [clang-format] Fixed AfterEnum handling.

I hadn't had another look, because the CI still shows the test AfterEnum to be failing.

Mar 8 2021, 8:43 PM · Restricted Project, Restricted Project

Mar 7 2021

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

Has anyone else had the chance to review this yet? (not trying to be pushy, just curious)

Mar 7 2021, 9:09 PM · Restricted Project, Restricted Project

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