MyDeveloperDay
https://mydeveloperday.wordpress.com/
twitter: @mydeveloperday
User Details
- User Since
- Aug 18 2015, 2:16 AM (359 w, 3 h)
- Roles
- Administrator
Yesterday
LGTM
LGTM
Sat, Jul 2
LGTM
Wed, Jun 29
LGTM
Sun, Jun 26
LGTM
Tue, Jun 21
Thanks for the Screenshot, please remember to mark your comments as "Done" once you've addressed them. From what I can tell this looks good.
Mon, Jun 20
Thu, Jun 16
I think you are missing a PARSE unit test
Wed, Jun 15
Thank you for the patch, just some observations
what about (may not be useful but compiles)
Sun, Jun 12
Great thanks
Sat, Jun 11
Thank you for the patch, was there a github issue for this? Can we just validate that those requirements are covered here too while we are at it?
We get heavily criticized for changing defaults between versions, I think if someone want to use GNU style, and they choose to now have this option set it should be done so in their own .clang-format file
If we remove it from here, then it will get removed from the documentation
Fri, Jun 10
Oh gosh! It hard to make such changes and someone else not call them a regression, I’m not sure how I feel. We do kind of have to be able to fix bugs and if this gets closer to gnu style then I guess it’s better
Mon, Jun 6
nice!
LGTM
Jun 1 2022
@stasm thank you for taking it on, "Commandeer" the revision via the Revision actions.
LGTM
May 23 2022
LGTM (sorry I've been slow on the reviews, my day job keeps getting in the way ;-))
LGTM too.. @owenpan this is one of my favourite features!!
This looks good to me, but let the others chime in..
May 21 2022
Brilliant thank you, LGTM
May 4 2022
You add significant number of keywords here but I don't see any of them being tested? can you add a unit tests to cover what you are adding
Is there any way you could add a unit test for this?
Apr 28 2022
Apr 27 2022
Apr 21 2022
As first passes for adding a new language I think this looks pretty good.
Apr 20 2022
Thanks for taking this on.
Apr 19 2022
Accepting but please wait for @owenpan
The hard part about doing 2 separate cases is where we miss-interpret a .hpp or .h file as being the wrong language (C/C++/ObjC/ObjC++), I don't think I have a problem with a single regex (its not like we change it that often).
Apr 8 2022
LGTM
Mar 27 2022
LGTM thank you for the patch
Mar 26 2022
Mar 23 2022
I'm a little uncomfortable with
Mar 22 2022
I hit something like this today with some code which was
FWIW I'm not a fan of the while( \n case, so assuming this fix, fixes that then that would be good I think.
It turned out this patch does change behavior.
Mar 21 2022
But we can: https://gcc.godbolt.org/z/8Mb1xo7oP
Mar 19 2022
Mar 18 2022
I think you can spin it any number of ways, but bottom line its a massive truth table and I sort of feel however you change it, it will just become a different equally incomprehensible pattern.
LGTM
Please wait for the other reviewers if you have commit access.
Sorry I don't believe we've covered the changes with unit tests.
This just made a 300 lines function and a 500 line function with minimal comments into a 800 line function.. For no real benefit?
Can you please supply full context diff "Context not available."
Wow! Now that’s what I’m talking about!! Awesome! Thank you so much!
I’m not blocking it just using the observation to highlight that the rst generated isn’t conformant with best practices. The solution being that a volunteer offers to fix the dump_format_style.py or enduring Format.h isn’t making those mistakes
Mar 17 2022
So out of interest, what is the reason? my assumption is that you wanted to add more for Verilog and you felt adding the extra bools was the wrong design and its better an an enum
Just an observation, I have this in my Day Job all the time..
Abandoned? huh? are you breaking it apart?
- Please log a github issue showing us what you are fixing.
- Please add the main clang-formatter developers as reviewers and not the clang-format project (I think we see it but I like to see the individual reviewers so I know there opinion)
- How about you change this so it autogenerates the rst so its never out of date.
Mar 16 2022
Can you add a summary as to why you are making this change? the code doesn't look the same before/after so whats changing and why? is there no unit tests that perhaps need to be added?
You need to rebase the patch, it doesn't apply.
I'm very much a "clang-format all the things" kind of person which is why I added C# and JSON support, as someone who moves between languages all day every day I want one tool to rule them all.. I'd love it if it would format YAML (but thats another story)..
Firstly thank you for taking the time to submit the patch, I've only taken a cursory view at first and I definitely have some comments, I've added the main clang-format reviewers
Mar 15 2022
Please do, to be honest I spent enough time on this and I got to the same conclusion, it needs a rethink. Please go ahead, I don't use this feature myself so I'm not likely to see all the issues.