- User Since
- Jul 10 2012, 10:15 AM (249 w, 6 d)
Sun, Apr 23
Fri, Apr 21
Thu, Apr 20
Tue, Apr 18
Sun, Apr 16
Tue, Apr 11
Mon, Apr 10
Yes, thank you!
Looks good. Thanks for cleaning this up.
Wed, Apr 5
Sat, Apr 1
This file is auto-generated from include/clang/format/Format.h. Changes need to be made there and then this files needs to be regenerated with docs/tools/dump_format_style.py. Sorry, we probably need to make that more obvious.
Fri, Mar 31
Thu, Mar 30
Generally please upload diffs with more contexts. For some here it's not even clear to which option they refer ;)
Tue, Mar 28
Mon, Mar 27
Thank you for working on this. Unfortunately, this is done at the wrong level:
- You are using a separate pass, which means that as soon as the preprocessor directives exceed the column limit, they won't be wrapped correctly.
- You aren't using Clang's Lexer to separate PP directives into tokens, which might work in the short term, but seems really fragile and a maintenance headache.
Sun, Mar 26
Mar 22 2017
Looks good.. Very nice :)
Mar 20 2017
Looks good, just minor remarks.
Mar 16 2017
Mar 13 2017
Mar 10 2017
Mar 9 2017
A few nits, otherwise looks good.
I think the patch is fine, except for the name of the flag. It is not breaking inheritance ;).
Mar 8 2017
Please upload patches with full context or use arc (https://secure.phabricator.com/book/phabricator/article/arcanist/).
Mar 7 2017
Looks good, thank you!
Minor nit, otherwise looks good.
Mar 6 2017
Would probably be interesting to add these test cases:
Looks good. Thank you for doing this!
Thanks for catching and fixing this.
Generally, please upload diffs with the full file as context. That way, phabricator allows expanding the code around your changes.
Mar 3 2017
Hm. Unfortunately, this seems to have been implemented to support Webkit style and Webkit style is explicit about wanting this (https://webkit.org/code-style-guidelines/) :(.
Do you know whether that is intentional? The style guide isn't really conclusive.
Before '? Can you give an example?
I agree, just generally we should aim for keeping these short.
Sure, then go ahead. If these examples would have helped you, that's one datapoint :).
Hm. I don't actually know whether these examples are useful as is. You only present one setting of the value in most cases. What's interesting is actually how the flag changes the behavior. I mean in most cases, this can be derived from the example, but in those cases, it's also fairly obvious what the flag does. Unfortunately, I also don't have a significantly better idea. Maybe something like https://clangformat.com/ is just better at handling this?
Mar 2 2017
So, while it might be convenient to view this all in one file, a test here is not convenient for me (or presumably other clang-format developers) to work with. You can make a pretty much 1:1 copy of it using a raw string literal in unittests. However, I don't think this is actually a good idea.
Please don't add this as is. I don't usually run the file-based tests in my development workflow and suspect that I might be breaking this a lot.
As discussed offline, I think this solves the wrong problem. My guess is that breakProtrudingToken checks State.Stack.back().NoLinebreak, but I forget to make it also check NoLinebreakInOperand.
Please include the reasoning in the patch description, i.e. that otherwise clang-format might need to runs to add all the namespace comments.
Mar 1 2017
Could you please upload a diff with the entire file as context? That makes reviewing this easier.
Feb 28 2017
Feb 27 2017
On by default for LLVM and Google style, don't know about the others.
Feb 26 2017
Feb 25 2017
Feb 23 2017
An unwrapped line can contain many braces, but (I think/hope) only one that belongs to a non-nested block. So every unwrapped line that opens a namespace/class/function/enum/... should have exactly one counterpart that closes it again.
I started reviewing, but after a while it occurred to me that some of this might not be the right approach. We are essentially duplicating the parsing of namespaces, which is already done in UnwrappedLineParser. Lets instead just store more information in the AnnotatedLines (or FormatTokens) to carry the required information forward.