Karthik Nayak <karthik.188@xxxxxxxxx> writes:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
>> Offtopic. After applying this topic, I asked clang-format if it
>> wants to change anything.
>>
>> $ git clang-format --diff $(git merge-base HEAD master)
>>
>> The result was disasterous. Can "clang-format --diff" mode be
>> taught a bit more focused to avoid touching existing entries in the
>> same array (in this case opts[] that has tons of options for the
>> "git for-each-ref" command), when only one new entry was added, I
>> wonder?
>>
>
> I couldn't find any way to do something like this.
>
>> Also I am not impressed by the change it made to the code that is
>> commented out (in refs.h).
>>
>> Line wrapping it did to refs_ref_iterator_begin() is an improvement,
>> but those to ref_iterator_seek() and do_for_each_ref_iterator() are
>> unnecessary (both of these were more readble in the original).
>>
>> Even though I found its output better for Toon's "last-modified"
>> changes, I am not impressed by what clang-format suggested for this
>> series.
>>
>
> It indeed looks really bad, I had a go with the new changes from
> 'gitster/kn/clang-format-updates'. Which seems a lot better.
>
> However, this does show a problem with using 'RemoveBracesLLVM', where
> it formats the following:
>
> if (...) {
> ...
> ...
> } else {
> ...
> }
>
> to:
>
> if (...) {
> ...
> ...
> } else
> ...
>
> Which isn't our style, I think we should completely drop this too, from
> my patch series. Let me go ahead and do that. I really want to strip out
> as many rules as possible to make the number of false positives 0 so we
> can actually start enforcing clang-format. Once we enforce it, we can
> slowly see what additional rules work well for us.
>
I did some more testing here, and it seems like this was because this
particular instance was more like
if (...) {
...
} else {
...
}
Where both the clauses had single line statement, but we only modified
the 'else' part of the clause in this patch series, so clang-format,
only suggested removing the braces from the 'else' clause.
So all is good here, I think we can go ahead with the
'gitster/kn/clang-format-updates' and merge it to 'next'. Sorry for
being the false positive, I thought I missed testing a particular case
and the series.
Attachment:
signature.asc
Description: PGP signature
