MyDeveloperDay
https://mydeveloperday.wordpress.com/
twitter: @mydeveloperday
User Details
- User Since
- Aug 18 2015, 2:16 AM (407 w, 2 d)
- Roles
- Administrator
Yesterday
LGTM
Mon, Jun 5
did you consider a case where the case falls through? (i.e. no return)
Apart from the documentation this looks fine.. thats probably outside the scope of this change
Fri, Jun 2
Thu, Jun 1
Wed, May 31
Sun, May 28
Fri, May 26
LGTM
Wed, May 24
I think we need to extract the context of the test from RenameTests to ensure we have it covered here. I don't personally normally run the entire LLVM suite.
We don't normally land broken tests, even if they are disabled, its better for us if we get a fix at the same time ;-)
Mon, May 22
Thank you for the indepth explaination in https://github.com/llvm/llvm-project/issues/59178, that was really helpful for me trying to understand what the problem was.
Tue, May 16
Sun, May 14
no concerns from the clang-format front.
Thu, May 11
Tue, May 9
Interesting
May 3 2023
ok! I'm not a massive lambda expert, but aren't we passing by const reference e.g. const FormatStyle &Style, can someone give me a lession as to why it thinks its by value? maybe I'm looking at the wrong "pass by"
Can you link to the Coverity issue so we can see what it was complaining about?
Thanks for the patch...this tells me people are starting to use this feature in anger!! i.e. your formatting via git-clang-format (which is brave!) ;-) which means you have the code modifying features perhaps on my default...
Just an update on this, it works well but some of the cases are not tolerant to comments, I'm working my way through those issues
May 1 2023
Could you add anotator tests?
Apr 28 2023
Apr 25 2023
is it possible to pass other arguments like -n if not running in place is there any protection from prevent stdout/stderr from overlapping..?
Apr 24 2023
Apr 20 2023
Handle the case
There is another case I need to cover
Address review comment, add convenience functions to simplify conditions
LGTM
Apr 19 2023
In my C# project, these settings give me just what we tend to use.
Sorry had a clang-format reflow comments moment, revert the unrelated comment changes
simplify the if's a little
Adds additional options to give us much greater control over how we format C# properties especially auto properties
There is more to this than meets the eye.. what we have so far, from existing AfterFunction use and the propsed here AfterCSharpProperty is...
Apr 18 2023
upload the correct patch file
add init support and fix indentation issue when only one property is defined but the is an auto-property
public Foo { set; get; }At least from my experience, the getter is specified before the setter, though I'm unsure how important this is in your eyes.
need to handle init;
need to handle public|private|internal accessors
Address review comments, still would like to solve the indenting issue.
Apr 17 2023
simplify negative if
Regarding the comment that I must not change existing tests: I think this rule is too strict, because those tests are mostly regression tests.
But a regression tests does not test for correctness. So if a test had already a wrong assumption, it must be changeable.
Cloud you include a test that contains multiple levels of nested scope, I'm assuming we won't add an additonal line at every {} level (or will we?)
I know it might not seem an obvious use case but there really isn't a requirement to not include header files more than once.. imagine if I have
Apr 16 2023
@exv any thoughts on this one? given that you were the author of D101860: [clang-format] Fix C# nullable-related errors and
@jbcoe as you were the author of D75983: [clang-format] Improved identification of C# nullables
The exclusions are not complete for example cond? foo() : "B"; would still fail. but this moves us a little closer
re clang-format
for default set;get or get;set for when AfterCSharpProperty is true,
I'm not convinced AfterCSharpProperty is a good name, open for suggestions
If this was the functionality pre 16 then LGTM
Apr 14 2023
Apr 13 2023
we should be good now
LGTM
Apr 12 2023
is it possible to point to a github issue that this related to in the review summary, if not please can you make one so we can track the issue its trying to solve
If all comments and concerns are done, then I'm inclined to accept, but I'd like @owenpan and @HazardyKnusperkeks to give their opinion before we land this.
Apr 11 2023
Apr 10 2023
Yes I didn’t see them repeated below
These features tend to have an additional performance penalty that not all may want to pay, even if they did base their style on your style. I.E. chromium isn’t likely the only ones using chromium style
This was the agreement reached when we pushed for clang format to start being allowed to add or rearrange tokens, I.E. in this case we add {} as such we could, as you saw yourselves make mistakes if our assumptions are not 100% correct and break code, we prefer people opt into this by turning it on in your .clang-format file
Sorry I don’t get how this change helps. Removing the option values doesn’t make it clearer IMHO