- User Since
- Feb 1 2020, 6:09 AM (50 w, 6 d)
Oct 15 2020
Belated 'sounds good to me'.
Sep 21 2020
I'd be very surprised if any of the tests included in this change pass with that line commented.... it's meant so that things like #if and #else properly separate alignment after the first preprocessor run, where the whitespace manager doesn't have the full context of things between it.
Sep 13 2020
After looking at more of the history (ie, the commit you referenced), I'd definitely be open to something like this, provided that it doesn't affect namespaces that reside completely on one line. Since it was mostly a clang-format limitation and relatively rare, I think we can change the default here, but that's not up to just me (+@MyDeveloperDay), and extra scrutiny is definitely required when changing existing tests.
Sep 12 2020
Sorry on the delay, LGTM too.
Sep 8 2020
Sep 7 2020
I can see the use of this, but I am also wary that ignoring style options will lead to people producing different results on different versions of clang-format. This is both because having set-or-unset an option will naturally lead to different code and also that newer options are a de-facto check that clang-format is at least a certain version (we have minor differences between major versions as bugs are fixed). In any case, I see this being *very* easy to misuse and the documentation should have a warning reflecting that.
Aug 27 2020
LGTM, again assuming tests pass locally (patch did not resolve).
LGTM, assuming tests pass (automated checks failed to resolve your patch since you based it off of your other one). Looks like enabling C99 should have no other effects, right?
LGTM. Wait a bit to give others a chance to chime in before submitting.
Aug 26 2020
Agreed that this is a very nice solution for this case. LGTM too assuming it passes @MyDeveloperDay's tests.
Aug 23 2020
Aug 17 2020
Reviving this since it looks perfectly fine to me (from my limited commit history in git-clang-format :P), is useful, and there's no good reason for it to be stalled.
Jul 18 2020
Jun 29 2020
Jun 26 2020
Thanks for the fast review, @curdeius, and thanks for mentioning PP_STRINGIZE and BOOST_PP_STRINGIZE too!
Address feedback (nits, better docs, more defaults)
Jun 25 2020
May 28 2020
Great idea on this! I may borrow this idea and make something similar for some migrations I'm working on.
May 25 2020
May 24 2020
May 23 2020
May 22 2020
I'm a fan of the 'like' helpers, but I'm not entirely convinced that having helpers for languages covered by the 'like' is a bad thing-- it just needs to be very explicit that you do mean only that language. For example, an 'isObjCOnly()' would hint to reviewers that ObjC is sometimes used in combination with other languages and there may be a more appropriate helper (of course, this should be clearly documented as well).
May 20 2020
Just belatedly caught something: Webkit style is supported too but not listed here. Can you add that?
May 19 2020
+1 for this idea. It'd eventually be neat to also take all samples from the style guide of each project and test them, if there aren't licensing concerns.
Hey @MyDeveloperDay, can I get your assistance committing this when you have the chance?
Can I get your assistance committing this?
Reformat with a newer clang-format
LGTM assuming CI tests pass.
May 18 2020
See follow-up in D80176.
This is a great improvement in readability. I think this will get in here before it does for clang proper :D
I mainly have concerns with the search for noexcept being too brittle and not looking far enough back. Implementing that may have performance implications though, so I have no issue ignoring that problem if it's proven sufficiently rare.
Good idea, feel free to incorporate anything about the change (eg, tests and TT_BitFieldColon) or delegate to me if you like.
Ironically, I independently made a near-identical change because my codebase needs this, and was going to upload it today. I won't post it now, but I stuck a copy on github if you're interested: https://github.com/JakeMerdichAMD/llvm-project/commit/6eec9ce0bb7732a519b765659422b72a9fa8aa67. I also have some more tests there, and will cross-check with this once the merge conflicts are fixed.
May 15 2020
Rebase to fix merge conflict
Add a comment explaining why checking IsInsideToken is needed here.
May 13 2020
One spelling nit, but otherwise looks good to me.
Thanks for the commit and review @MyDeveloperDay!
May 11 2020
Hey, @MyDeveloperDay, can I get your assistance in committing this? It's probably been long enough for anyone to chime in.
May 6 2020
Mind looking again? They did add one later on I think. It helped a lot since the exact settings and whitespace to trigger this are really finicky (though reproducible). You're right that it won't reproduce without it.
May 5 2020
The failing case in this commit looks like the following after formatting (with alignconsecutiveassignments and a specific column limit)
Sure, I'll get started on that. It mainly comes from charging headfirst into the edge cases, but I think I have a decent grasp of clang-format internals now, and I'm definitely interested in both contributing to and reviewing for it.
@MyDeveloperDay, you're right about what this issue addresses: it surprised me a lot when an unrelated edit caused something to 'randomly' add spaces elsewhere, since it's in a different ifdef block (whether or not it has the same condition). The code that's aligned should always be across lines that are visually consecutive, and those ones weren't (in most cases, they aren't even logically consecutive).