- User Since
- Feb 1 2020, 6:09 AM (22 w, 5 d)
Mon, Jun 29
Fri, Jun 26
Thanks for the fast review, @curdeius, and thanks for mentioning PP_STRINGIZE and BOOST_PP_STRINGIZE too!
Address feedback (nits, better docs, more defaults)
Thu, Jun 25
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).