- User Since
- Aug 18 2015, 2:16 AM (270 w, 1 d)
I think this patch needs rebasing
LGTM, I'm for anything that speeds up the build
Fri, Oct 16
@catskul I think @STL_MSFT gives justification enough, I'm personally not of the belief there is one true format. I like clang-format to be flexible enough to meet the requirements that @STL_MSFT says, I also don't like @clang-format being the source of friction, and I don't like its constraints being the thing that prevents people from switching to clang-format.
LGTM, only keep the tests if they remain passing, otherwise perhaps @JakeMerdichAMD can add them back in.
Wed, Oct 14
Fri, Oct 9
Not to my knowledge, if we are not going to fix it we should probably revert it for now
Wed, Oct 7
I have noticed that clang-format has a tendency to over eagerly make * and & a TT_BinaryOperator, I know its too late to change it now, but sometimes I wonder if they should be detected as TT_PointerOrReference and then the clauses try and expose when they are being used as BinaryOperators we might have better luck.. or maybe not.
Fri, Sep 25
Which bit to you find more complex? adding something to the FormatToken or the use of the is() calls?
This LGTM, generally I think clang-format seems to remove trailing spaces anyway so it might be that the duplicate might disappear on the second format. but I'm good with this approach
Thu, Sep 24
@oleg.smolsky I agree, what you have here covers a myriad of other cases and was committed in 2018, we can't call this commit a regression it was a feature ;-), if we want to improve the feature that is something else.
I think if you have a proposal for changing the behavior lets see the patch and how it impacts the existing unit tests
Tue, Sep 22
I noticed the pre-merge tests failed!
In the examples you give here.. (and I have a feeling there are others in the tests) some of these fields are the same in all 3 cases
Sep 21 2020
Sep 18 2020
it certainly LGTM, like I said I've had met this issue before and I think this does get around that situation where perhaps your not even using the option it's complaining about (especially if its a C++ option and you are using clang-format for C#)
IMHO I feel that given that these changes are more wide reaching than just clang-format that it might be more correct for your to request commit access yourself.
Sep 17 2020
Sep 16 2020
Sep 15 2020
Firstly let me say thank you for the patch, and taking the time to listen to the reviewers. Please don't be discouraged by my reply/opinon.
I have realized this is the wrong change
Sep 14 2020
I have a hard time when people change tests! just because one person wants them this way doesn't mean everyone will.
Sep 13 2020
Minimize other places where try could be incorrectly seen as an identifier
Sep 9 2020
LGTM, thank you for doing this..
LGTM, I'm happy if @JakeMerdichAMD is
Sep 8 2020
Note to self, other things could cause the same issue
@Saldivarcher this should be landed now, please validate
Better solution here D86581: [clang-format] Handle shifts within conditions
Sep 7 2020
Thanks for the patch, I think this looks like a comprehensive improvement, a new nits only
Regarding not touching the LLVM support library: I'd love to find a way, but as clang-format uses the >> operator
Thanks for the patch, You need to generate a fill context diff (see Contributing to LLVM)
Sep 5 2020
This has caught me out from time to time, but the presence of such an option can lead to users mixing clang-format versions which could lead to flip/flopping of changes, so I'm not 100% sure it won't drive bad behaviour.
Sep 4 2020
LGTM, I think we need something like this for those keywords/macros
Assuming it builds with minimum required c++ compiler
I'm not a C++ language lawyer so I'm not sure which standard introduce enum inheritance? (maybe its something we could always do?), if its a recent change we just need to ensure we are conforming to the C++ version that is needed to build clang.
This LGTM, I've certainly been bit myself in the past with not knowing which test actually failed especially when they are all foo() something.. thank you
Sep 2 2020
Sep 1 2020
I'm wondering if this could be used to help the CUDA usages of clang-format
Aug 28 2020
This LGTM thank you
LGTM thank you for these updates
Aug 27 2020
@Saldivarcher do you have commit access?
I think this LGTM and the solution is much more elegant than mine! You were not stepping on my toes, I'm always happy for someone else to take things over and this is a much smarter solution than mine.
Aug 26 2020
I think I like your solution better than mine D79293: [clang-format] [PR45218] Fix an issue where < and > and >> in a for loop gets incorrectly interpreted at a TemplateOpener/Closer, could you look at my unit tests and consider adding them here?
Jul 28 2020
Thank you for the patch, this LGTM, I think this kind of change should help reduce memory usage and I feel improves the readability.
Jul 27 2020
a new minor Nits but looks good.
Jul 23 2020
Thank you for the patch, I'm generally in agreement but I think we should let some others comment
Jul 21 2020
just reopening to formally accept
Thanks for assistance and review. And sorry about the extra noise.
Jul 20 2020
Nit:clang-format the patch
Thanks for doing this..
Jul 19 2020
Let’s do all 4, None, Both, Left, Right
Jul 18 2020
you need to make a full contextdiff git diff -U 99999999