- User Since
- Aug 18 2015, 2:16 AM (265 w, 5 d)
Fri, Sep 18
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.
Thu, Sep 17
Wed, Sep 16
Tue, Sep 15
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
Mon, Sep 14
I have a hard time when people change tests! just because one person wants them this way doesn't mean everyone will.
Sun, Sep 13
Minimize other places where try could be incorrectly seen as an identifier
Wed, Sep 9
LGTM, thank you for doing this..
LGTM, I'm happy if @JakeMerdichAMD is
Tue, Sep 8
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
Mon, Sep 7
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)
Sat, Sep 5
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.
Fri, Sep 4
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
Wed, Sep 2
Tue, Sep 1
I'm wondering if this could be used to help the CUDA usages of clang-format
Fri, Aug 28
This LGTM thank you
LGTM thank you for these updates
Thu, Aug 27
@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.
Wed, Aug 26
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
In principle, this looks ok.
Jul 16 2020
clang-format -n warnings before this change in clang/test/Analysis/*.cpp (clang-format -n *.cpp |& grep warning | wc -l)
Jul 13 2020
Jul 12 2020
Apologies our clang-format fix D83564: [clang-format] PR46609 clang-format does not obey `PointerAlignment: Right` for ellipsis in declarator for pack broke the buildbots for Polly. Hope this is ok to fix this.
Jul 10 2020
Jul 9 2020
This issue is caused by the presence of the ; in the requires clause (0)>;) inside the template declaration
Jul 7 2020
https://reviews.llvm.org/D79773#2131680 has something to do with your .clang-format file, the base LLVM style seems fine.
Jul 6 2020
We may not be consistent across all of LLVM
Thank you @JohelEGP please keep them coming, I'll try and update the patch as soon as I have some fixes, I kind of feel like we are addressing what would have just been downstream bugs anyway. (some are quite bizarre!) so lets squish as many of them as we can before we land this.
Jul 3 2020
Addressed review comments
Added one or two more tests
Jul 2 2020
Still a work in progress but covers I think the case @JohelEGP found.
Jul 1 2020
I'd be interested to know if you felt any of those changes were an unwanted surprise
Thank you I see now... super subtle but I'll take a look.