- User Since
- Aug 18 2015, 2:16 AM (187 w, 4 d)
@lebedev.ri Are we talking about a general ideology of the long term cost to allow any new things in? or to not allow things in this specific case?
Thu, Mar 21
Address comment nits
Addressing Review Comments
- Remove all non Format only code (i.e. changes to clang/Basic and clang/Lexer to support verbatim interpolated strings C# 6)
- For the most part the need to for them is removed by merging of tokens during the FormatLexer phase
- There is one obscure failure scenario, but for a first pass I think it can be ignored (tests commented out) where a \ is used unescaped just before a " (because of paths can be more common than you think)
- string s = @"ABC\" + ToString("B");";
Wed, Mar 20
The cost is financial, as it's developer time, which costs real money to companies. In the end, to support this, people like myself who are doing this as part of their job spend time that they'd otherwise spend to make other things better that might be more important.
Sorry for the spam:
- write this a little better
Addressing code review comment
@klimek I think is what you meant, a much smaller and neater version of what I had before.
Tue, Mar 19
Address review comments
This may not be a more satisfactory solution but it at least now covers the other cases raised by @JonasToth
Add more tests around this area.
Minor modification to improve the [=] case
(landed here too https://github.com/mydeveloperday/clang-experimental/releases)
Mon, Mar 18
I don't think risk is what matters - in the end it's about cost, and cost is a very technical reason
Sun, Mar 17
Sat, Mar 16
I'm sorry for not having a positive answer here, but I'm not in favor of this change. The style rule looks like it introduces arbitrary complexity at a point where I don't understand at all how it matters. We cannot possibly support all style guides that come up with random variance. I do not think this option pulls its weight.
I'm happy to be wrong, but In current master, SpaceBeforeCpp11BracedList is a boolean not an enum (but I think I've seen a review changing it somewhere which maybe isn't landed)
Fri, Mar 15
nobody being able to make changes
So @russellmcc you've been bumping along this road nicely for 6 months, doing what people say... pinging every week or so in order to get your code reviewed, and you are getting no response.
From what I can tell this looks OK as you seem to be mainly only passing down the boolean, my guess is that while most people don't use the -lines directly that often but it probably gets called by some of the formatting tools that work over selections, maybe something like git-clang-format
Thu, Mar 14
Remove non related clang-format changes
run git clang-format
Add a Microsoft style (trying to match most closely the more traditional Microsoft style used for C#)
This will allow, CSharp to be added to the config by
@to-miz do you have permission to commit?
Pulling this revision down to build it caused build issues with missing SBBLO_Always, could you investigate before committing?
of course, this isn't legal
That works because the argument list is just tok::identifier and TT_StartOfName
Wed, Mar 13
sorry incorrect files in the merge reverting...
Tue, Mar 12
run git clang-format
Mon, Mar 11
most of the cases that we were adding here were for a templated return types
Address review comment
- add document and comment spelling and punctuation
- add additional unit tests to show non compound else clause case
Address review comments
- collapse two options into one
Sun, Mar 10
Adding the unit tests lets us see how this option will work in various cases, it will let us understand that its not breaking anything else.
Sat, Mar 9
Please add unit tests to show this working, otherwise later someone will come along and break the behavior
This review lack unit tests, please add some for FormatTest to show its use cases, otherwise someone else will come along and break this later
This review needs unit tests
I'm not the code owner but this LGTM,
Fri, Mar 8
Fix spelling typo in documentation and comment
Add missing Format.h from the review
Thu, Mar 7
I definately been burnt by not handling all the cases, but imagine a string class
Tue, Mar 5
Mon, Mar 4
I'm not sure I personally would ever write code like that ;-) , but if its legal C++ and it compiles we should handle it the same as foo<1>,foo<true>,foo<!true>
It might be a good idea to add the nested lambda tests from D44609: [Clang-Format] New option BeforeLambdaBody to manage lambda line break inside function parameter call (in Allman style) into your review to show how this patch copes with those (and the examples from the comments of that review)
Sat, Mar 2
Thank you for helping to address one of the many clang-format issues from bugzilla, I'm not the code owner here but this looks good to me. If we want to address the many issues we need to be able to move forwards.
Fri, Mar 1
Fix negative test case
support the same /*<space>clang-format off<space>*/ in the sort includes that the TokenLexer supports.
Thu, Feb 28
sorry to be critical, just trying to help get traction on your review...
I'm trying to go over old reviews and see if they are still desired, I stumbled on your review and want to see if its still important. Here is some initial feedback.