- User Since
- Jul 29 2013, 1:59 AM (412 w, 16 h)
Wed, Jun 16
Wed, Jun 9
LGTM. That's a great piece work @feg208. Thank you!
Tue, Jun 8
LGTM modulo nits.
Sun, Jun 6
This has been superseded by D103245. Closing.
Fri, Jun 4
Thu, Jun 3
LGTM. Thanks for fixing this!
Do you have commit rights or you need somebody to land it for you?
Wed, Jun 2
Thanks for looking into this, but please add a test case that demonstrates the bug to FormatTests.cpp
Also, is there, by any chance, an associated bugzilla ticket?
Thanks for adding the tests. 👍
Sat, May 29
LGTM. But please wait for @HazardyKnusperkeks to land.
Fri, May 28
Thanks for working on this!
Looks pretty nice, but please clang-format the changes and add some more test cases (cf. inline comment).
Seems really nice. Just some nits & question.
Thanks for resuscitating the other review!
May 19 2021
May 14 2021
Also, please update https://bugs.llvm.org/show_bug.cgi?id=48939 and add it to the description.
LGTM. But please rebase to rerun CI (the commit that provoked the failures has been reverted).
It would be cool if @saugustine could have a look at gdb part though. @ldionne, this patch should unblock adding 32-bit buildbots to CI in D92508.
May 13 2021
LGTM. What do you think, would there be any value in testing different char types?
LGTM. I second what Arthur said.
LGTM. Not blocking, but I'd add foo4 example from the description as a test case (with a link to the bug maybe).
May 12 2021
May 7 2021
May 6 2021
May 5 2021
Oh, I forgot, please add a release note.
LGTM but let's wait a day or two before landing, so that others can chime in.
Btw, do you have commit rights? If no, please provide "Name <email>" for commit attribution.
I like the idea, but we need more tests.
LGTM pending CI.
LGTM pending CI.
You can add a parent revision so that this patch will be applied upon its parent.
Could you please rebase to re-run the CI (that failed for sum unrelated reason)?
May 4 2021
Preferably a new one unless it's something really minor.
LGTM. That's great. Thanks for working on this and for the cleanup.
May 3 2021
Looks very good already. Just some questions and test requests.
Is it testable?
- Fix format.
May 1 2021
I went a different way in order to keep backward compatibility.
I renamed Alwayss to OnlyFirstIf and added AllIfsAndElse that behaves as Always in my initial patch.
@MyDeveloperDay, please have another look.
- [clang-format] Apply AllowShortIfStatementsOnASingleLine: AllIfsAndElse to "else if" and "else". Keep backward-compatibility.
Apr 30 2021
LGTM pending CI.
Apr 29 2021
Apr 28 2021
LGTM. Is there any other option on clang-cl to avoid linking MSVCRT but linking other stuff? Is there going to be any change in clang 13?
Apr 27 2021
LGTM. I have no strong feelings about the order of commands (per Arthur's comment). Both are OK for me.
LGTM. I also consider it a bug. LLVM should not be affected as it uses AllowShortEnumsOnASingleLine: true whereas this problem arises only with AllowShortEnumsOnASingleLine: false.
Anyway, those that find the new behaviour problematic, can always set BreakBeforeBraces: Custom and BraceWrapping.AfterEnum: true to get the old behaviour.
@lunasorcery, please update release notes before landing.
If you don't have commit rights, please provide "Your Name <email@address>" for commit attribution, otherwise we'll use the name/email associated with you Phabricator account.
Apr 24 2021
LGTM. Please update the filename in the comment as indicated by Arthur before landing.
Stupid question... but what's SMF?
Apr 21 2021
Apr 20 2021
LGTM. IIUC these tests will unexpectedly pass when this bug is fixed in clang, right? And then, we would be able to remove XFAIL annotations. If it's so, then it's good.
Apr 19 2021
More I look at the current state of this option, more I think it's misleading.
I would expect that the if after else is also controlled by this option. And, the last else as well (adding yet another option for that seems to be a bit an overkill)