- User Since
- Aug 18 2015, 2:16 AM (283 w, 3 d)
Tue, Jan 19
Thanks for explaining! I visited the his profile. It is really confusing.
Mon, Jan 18
Similarly I see users having to define everything just to turn one thing off
I think this is LGTM, however..
What I can't easily tell from the tests is if you are overriding any styles defined in the parent with a local style.
Sun, Jan 17
I think the revision whilst it does what is needed to structs doesn't address the many other times this forms appear. I think we need something a little more extensive. It can't just be when a line starts with struct
Please address the "not done" comment (regarding the sorting), but other than that its LGTM
Sorry but due to the following issue raised by @RatTac , I'm reverting this prior to the LLVM 12 branch out so I can work on it further.
Thank you for changing the dump_style to make this clearer
Thie LGTM, just to check a few nits
Thu, Jan 14
My assumption is that you want to stick with the minimum and maximum is that correct?
Wed, Jan 13
I think I would remove the code examples from the "AlignConsecutive style" to avoid confusion (that would be the first change)
Sun, Jan 10
Thu, Jan 7
@RatTac Would you please consider copying the comment onto the review D92753: [clang-format] Add IndentPragma style to eliminate common clang-format off scenario and not the commit, I have a reply but don't want to write it here.
Tue, Jan 5
Thank you for submitting this patch?
Mon, Jan 4
This LGTM, I'm not sure if others have any further comments
Sun, Jan 3
I'm sorry, but now you are missing the files from the review, I think you diffing against your own commits and not commit that are in the repo. This makes the review very difficult to understand.
we don't commit with failing tests so lets understand why it fails.
by and large, this looks pretty good to me..
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
Sat, Jan 2
I think we are missing some clarity in this bug as to what the actual problem is, I do agree the test looks wrong,
Fri, Jan 1
Thu, Dec 31
ok so this looks ok, but before we commit can we have a discussion about why you think it fails for the comment case?
Take the following example:
I think if you think you have a bug that you log it in the bug tracker and we track it with this issue.
Wed, Dec 30
Address review comments
Tue, Dec 29
@mitchell-stellar do you plan to bring this back?
I like what you are suggesting, do you think BasedOnStyle: File is the best terminology as the term File is used elsewhere to mean read from the .clang_format file, how about
Mon, Dec 28
Addressing additional usecase found issues in C# tests too
Sun, Dec 27
I'm a little confused why this is needed as clang-format always read up the directory tree until it see a .clang-format file, perhaps I don't quite understand the use case. Can't you just have a different .clang-format in the subdirectory?
Thank you for this patch, a few process issues.
Sat, Dec 26
This is the closest I could find thus far http://bitsavers.informatik.uni-stuttgart.de/pdf/chromatics/CGC_7900_C_Programmers_Manual_Mar82.pdf
Thu, Dec 24
Wed, Dec 23
I don't have any major issues with this other than it makes Format.h a bit more ugly. (there are more ugly and harder to understand pieces of code than this change!)
We should probably check the docs/tools/dump_format_style.py still works
Dec 22 2020
remove = and ~ cases
remove unrelated change
Dec 21 2020
Dec 19 2020
Thanks for making the changes, this LGTM
With my maintainer-of-Support/JSON hat on, I don't like the idea of these features being shoehorned into the library to make clang-format an incrementally more featureful JSON formatter. They're likely to lead to a lot of conceptual+implementation+API complexity in a library that solves its primary use cases quite well and simply.