[llvm-dev] [RFC] Semi-Automatic clang-format of files with low frequency

[Hal Finkel via llvm-dev]

On 6/30/20 1:19 PM, MyDeveloper Day via llvm-dev wrote:
> I 100% get that we might not like the decisions clang-format is 
> making, but how does one overcome this when adding new code? The 
> pre-merge checks enforce clang-formatting before commit and that's a 
> common review comment anyway for those who didn't join the pre-merge 
> checking group.


Those checks are advisory. I do not recall a case where 
thoughtfully-hand-formatted code was proposed, adhering to the coding 
guidelines, and a reviewer asked for the file to be run through 
clang-format. When the thoughtfully-formatted code does not match what 
clang-format would produce in such cases, it's generally obvious why. 
Normally, this request is made where the file is inconsistently 
formatted, or formatted in a way that obviously does not comply with our 
guidelines.

I've observed that there is a wide spectrum of developer preferences: 
some thoughtfully and deliberately format all of their code, others deal 
with formatting only when they feel like they absolutely must and are 
happy to have a tool that does anything consistent. Most people are 
probably somewhere in between.


> I'm just wondering are we not all following the same guidelines?


We have specific guidelines that everyone should follow. clang-format 
implements a superset of those.

Here's a counter-proposal: You should hook up a tool that automatically 
opens Phabricator reviews to clang-format the source code. It should do 
this at a very low rate. It may take years to get through everything. 
But, humans will look at the formatting changes, and where clang-format 
should be improved instead of changing the code, we'll discover and 
classify those things. Formatted files can be noted, however, to 
incrementally build the clang-format test suite.


>
> Concerns of clang-format not being good enough are fair enough, but 
> that's the area I'm focusing my development efforts on (as that's 
> where I've been trying to make a small contribution). This effort was 
> driven out of a need in my view to have an extensive test suite to 
> validate new changes to clang-format don't break the formatting of 
> pre-formatted code. This isn't that possible with LLVM at the moment 
> because large parts are not formatted.
>
> However checking a code base which is in high flux but one that 
> maintains a 100% clang-format clean status, is a near perfect 
> test-suite. Especially one that I assume will have unit tests for the 
> latest C++ features.
>
> I'm not bored ;-) and whilst I understand this feels somewhat 
> periphery to the seemingly much more pressing task of developing the 
> next best compiler, I think we have to remember that clang-format is 
> extensively used. (In my view by many more people than those actually 
> using clang). GitHub reports 32,000 YAML files with the BasedOnStyle: 
> LLVM alone that is present in the .clang-format file


Independent of everything else, this is a great success. Maybe we should 
have a website with a tracker on it or something.

Thanks again,

Hal


>
> I realize it feels like unnecessary churn which is why I suggested 
> this be in code which hasn't been touched in some time (I'd be fine if 
> that time is 1+ years), but more often than not this is quite small 
> basic style issues, similar to the ones below.
>
> MyDeveloperDay
>
> -  void HandleTranslationUnit(ASTContext& context) override {
> +  void HandleTranslationUnit(ASTContext &context) override {
>      if (!Instance.getLangOpts().DelayedTemplateParsing)
>        return;
>
> -      std::set<FunctionDecl*> LateParsedDecls;
> +      std::set<FunctionDecl *> LateParsedDecls;
>
> On Tue, Jun 30, 2020 at 6:55 PM Matt Arsenault <arsenm2 at gmail.com 
> <mailto:arsenm2 at gmail.com>> wrote:
>
>
>
>>     On Jun 28, 2020, at 11:30, MyDeveloper Day via llvm-dev
>>     <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote:
>>
>>     I’m a contributor to clang-format and would like to see LLVM 100%
>>     clang formatted so we can use LLVM as a massive test-suite for
>>     clang-format when we make changes.
>>
>>
>
>     My main issue with this would be that clang-format does things
>     that I don’t believe are stated in the LLVM style guide and I also
>     disagree with. There’s a whole set of cases where it makes
>     unwelcome changes to put things that were on separate lines on a
>     single line. Whenever I run clang-format, I typically git checkout
>     -p to discard all of these.
>
>     For example, it likes to take member initializer lists and pack
>     them into as few lines as possible. This is just asking for merge
>     conflicts (e.g. AMDGPUSubtarget has a ton of fields in it, and out
>     of tree code is constantly adding new fields for unreleased
>     subtargets). It also mangles BuildMI calls, where I believe every
>     .add() should be on its own line, and stringing this into
>     BuildMI().addFoo().addBar() is way less readable.
>
>     I also believe it’s 4 space indent on line wraps differs from the
>     stated 2 space indent level (and this also disagrees with emacs
>     llvm-style)
>
>     -Matt
>
>
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

-- 
Hal Finkel
Lead, Compiler Technology and Programming Languages
Leadership Computing Facility
Argonne National Laboratory

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200630/e53127cd/attachment.html>