Re: [Yade-dev] clang-format (Was: Re: Removing trailing white space, yay or nay?)

[Anton Gladky]

Hi Bruno,

I think Janek has already answered most of the questions.

> What would be the workflow in general for, let's say, a kdevelop/kate user?

It is supported by most of IDEs, as it is getting to be a standard
tool for industry. The command-line tool will work always though.

> I'm particularly curious about your statement in the giltab thread [1]: 
> "Reformatting everything is not the best idea. It will change everything and 
> can hurt the history. I would propose to do it step by step, only by changing 
> the files."

> Do you suggest that each time a file will be modified it should be also 
> re-formatted, but no systematic reformatting would be done beyond that?
> Isn't it maximizing the risk of mixing real changes with formatting? I would 
> think reformatting everything in one single commit would make the history 
> much more clear.

I do not have a very strong opinion about that. If we do a
one-shot-reformatting - it is also OK. But I would then prefer
to set the author of this commit, something to "clang-formatter",
Just to identify, that this particular change was done by this action.

For example, my IDE shows at each line, who was the last author
of the particular line  (git blame basically). Sometimes it is useful to contact
the author of the line/code personally.

If we decide to use clang-formatter for the project - it will be
better to have it in all IDEs activated permanently. And "on-save"
the IDE will reformat it. It works in most of cases just fine. And
after some time you see, that you feel uncomfortable, if the project
is not using it  :) And yes, such flame-war questions like tabs/spaces,
width, position of braces, trailing whitespaces etc are disappearing
completely.

But again, it is up to the majority to decide, whether to use this tool or not.
But the code is getting more readable, more uniform and professional

Best regards

Anton

Am Mo., 21. Okt. 2019 um 11:57 Uhr schrieb Bruno Chareyre
:


>
> Hi Anton,
> It's not yet all clear to me how it will work.
> What would be the workflow in general for, let's say, a kdevelop/kate user?
> Edit, then "git-clang-format" before commit?
>
> I'm particularly curious about your statement in the giltab thread [1]: 
> "Reformatting everything is not the best idea. It will change everything and 
> can hurt the history. I would propose to do it step by step, only by changing 
> the files."
>
> Do you suggest that each time a file will be modified it should be also 
> re-formatted, but no systematic reformatting would be done beyond that?
> Isn't it maximizing the risk of mixing real changes with formatting? I would 
> think reformatting everything in one single commit would make the history 
> much more clear.
>
> Last thing I'm thinking about: how will we avoid that different authors with 
> (even slightly) different editing tools and auto-formating settings end up in 
> committing conflicting auto-formats?
>
> Bruno
>
> [1] https://gitlab.com/yade-dev/trunk/merge_requests/298#note_233001456
>
>
>
> On Sun, 20 Oct 2019 at 21:47, Anton Gladky  wrote:
>>
>> Dear all,
>>
>> there are several pre-defined styles in the clang-formatter.
>> I have recursively run all of them in the current trunk and I controlled,
>> how many lines were changed (+ lines added, - lines removed)
>>
>>
>> Style | +  | -   | Line length
>> 
>> LLVM  | 95796  | 71398   | 80
>> Google| 95748  | 72468   | 80
>> Chromium  | 104646 | 72689   | ?
>> Mozilla   | 109952 | 70635   | 80
>> WebKit| 76487  | 69974 * | ?
>> Microsoft | 97828  | 73115   | ?
>>
>> As you can see the minimal diff was produced by the
>> WebKit-style. It can be caused by the fact, that some
>> styles are changing the line length of the code. So,
>> this parameter is also in the table.
>>
>> After that I have took the WebKit style and played with
>> the type of indents: spaces, tabs (Always and ForIndentation).
>>
>> UseTab  | IndentWidth  | +  | -
>> 
>> Never   | 1| 77654  | 71141
>> Never   | 2| 73701  | 67188
>> Never   | 4| 76487  | 69974
>> Never   | 6| 78023  | 71510
>> Never   | 8| 77785  | 71272
>> ForIndentation  | 1| 77624  | 7
>> ForIndentation  | 2| 73336  | 66823
>> ForIndentation  | 4| 74727  | 68214
>> ForIndentation  | 6| 77538  | 71025
>> ForIndentation  | 8| 63022  | 56509 *
>> Always  | 1| 77623  | 71110
>> Always  | 2| 73340  | 66827
>> Always  | 4| 74722  | 68209
>> Always  | 8| 63013  | 56500
>>
>> I think that "tabs" won. Also "always" looks like too
>> restrictive, so I have chosen this parameter with the
>> indentwidth 8 as the minimal invasive.
>>
>> The last one is the maximal line length:
>>
>> ColumnLimit   | +  |

Re: [Yade-dev] clang-format (Was: Re: Removing trailing white space, yay or nay?)

[Bruno Chareyre]

Thanks for clarification Janek,
Yes, apparently kate and kedevelop5 [1] both have clang plugins.
Might be a bit more involved to get it working on 16.04 (there's kdevelop4)
but it should be doable.
Bruno
[1] https://www.kdevelop.org/news/first-beta-release-kdevelop-500-available


On Mon, 21 Oct 2019 at 14:53, Janek Kozicki (yade) 
wrote:

> Bruno Chareyre said: (by the date of Mon, 21 Oct 2019 11:57:28 +0200)
>
> > Hi Anton,
> > It's not yet all clear to me how it will work.
> > What would be the workflow in general for, let's say, a kdevelop/kate
> user?
> > Edit, then "git-clang-format" before commit?
>
> I pushed the script `scripts/clang-formatter.sh` which you can call on
> file or a directory and it will do the formatting. The script checks
> beforehand if there are any uncommitted changes, so that you do not
> mix substantial changes with formatting changes. Mixing them in
> single commit might cause problems with reading history, I suppose.
>
> > I'm particularly curious about your statement in the giltab thread [1]:
> > "Reformatting everything is not the best idea. It will change everything
> > and can hurt the history. I would propose to do it step by step, only by
> > changing the files."
> >
> > Do you suggest that each time a file will be modified it should be also
> > re-formatted, but no systematic reformatting would be done beyond that?
> > Isn't it maximizing the risk of mixing real changes with formatting? I
> > would think reformatting everything in one single commit would make the
> > history much more clear.
>
> Honestly I don't know. I think that at least for start we should just
> try to get comfortable with the new system. If we agree that it works for
> everyone then maybe we can reformat everything.
>
> clang-format appears to be a quite popular tool. I figured out how to
> invoke it from inside vim on the selected text [1]. I am pretty sure you
> can do the same from kdevelop/kate.
>
>
> > Last thing I'm thinking about: how will we avoid that different
> > authors with (even slightly) different editing tools and
> > auto-formating settings end up in committing conflicting
> > auto-formats?
>
> The basic assumption is that once we have a .clang-format file, the
> script `scripts/clang-formatter.sh` produces the same results for everyone.
>
> So even if you mix the formatting somehow, this script will reset it
> to what is in the repository.
>
> However I didn't yet check if the results on ubuntu 16.04 are the
> same as on debian bullseye. We better check that before a
> full-rollout.
>
> Janek
>
> [1] https://gitlab.com/yade-dev/trunk/merge_requests/298#note_232967339
>
> ___
> Mailing list: https://launchpad.net/~yade-dev
> Post to : yade-dev@lists.launchpad.net
> Unsubscribe : https://launchpad.net/~yade-dev
> More help   : https://help.launchpad.net/ListHelp
>
>
>

-- 
-- 
___
Bruno Chareyre
Associate Professor
ENSE³ - Grenoble INP
Lab. 3SR
BP 53
38041 Grenoble cedex 9
Tél : +33 4 56 52 86 21


Email too brief?
Here's why: email charter

___
Mailing list: https://launchpad.net/~yade-dev
Post to : yade-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~yade-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Yade-dev] clang-format (Was: Re: Removing trailing white space, yay or nay?)

[Janek Kozicki (yade)]

Bruno Chareyre said: (by the date of Mon, 21 Oct 2019 11:57:28 +0200)

> Hi Anton,
> It's not yet all clear to me how it will work.
> What would be the workflow in general for, let's say, a kdevelop/kate user?
> Edit, then "git-clang-format" before commit?

I pushed the script `scripts/clang-formatter.sh` which you can call on
file or a directory and it will do the formatting. The script checks
beforehand if there are any uncommitted changes, so that you do not
mix substantial changes with formatting changes. Mixing them in
single commit might cause problems with reading history, I suppose.

> I'm particularly curious about your statement in the giltab thread [1]:
> "Reformatting everything is not the best idea. It will change everything
> and can hurt the history. I would propose to do it step by step, only by
> changing the files."
> 
> Do you suggest that each time a file will be modified it should be also
> re-formatted, but no systematic reformatting would be done beyond that?
> Isn't it maximizing the risk of mixing real changes with formatting? I
> would think reformatting everything in one single commit would make the
> history much more clear.

Honestly I don't know. I think that at least for start we should just
try to get comfortable with the new system. If we agree that it works for
everyone then maybe we can reformat everything.

clang-format appears to be a quite popular tool. I figured out how to
invoke it from inside vim on the selected text [1]. I am pretty sure you
can do the same from kdevelop/kate.

 
> Last thing I'm thinking about: how will we avoid that different
> authors with (even slightly) different editing tools and
> auto-formating settings end up in committing conflicting
> auto-formats?

The basic assumption is that once we have a .clang-format file, the
script `scripts/clang-formatter.sh` produces the same results for everyone.

So even if you mix the formatting somehow, this script will reset it
to what is in the repository.

However I didn't yet check if the results on ubuntu 16.04 are the
same as on debian bullseye. We better check that before a
full-rollout.

Janek

[1] https://gitlab.com/yade-dev/trunk/merge_requests/298#note_232967339

___
Mailing list: https://launchpad.net/~yade-dev
Post to : yade-dev@lists.launchpad.net
Unsubscribe : https://launchpad.net/~yade-dev
More help   : https://help.launchpad.net/ListHelp

Re: [Yade-dev] clang-format (Was: Re: Removing trailing white space, yay or nay?)

[Bruno Chareyre]

Hi Anton,
It's not yet all clear to me how it will work.
What would be the workflow in general for, let's say, a kdevelop/kate user?
Edit, then "git-clang-format" before commit?

I'm particularly curious about your statement in the giltab thread [1]:
"Reformatting everything is not the best idea. It will change everything
and can hurt the history. I would propose to do it step by step, only by
changing the files."

Do you suggest that each time a file will be modified it should be also
re-formatted, but no systematic reformatting would be done beyond that?
Isn't it maximizing the risk of mixing real changes with formatting? I
would think reformatting everything in one single commit would make the
history much more clear.

Last thing I'm thinking about: how will we avoid that different authors
with (even slightly) different editing tools and auto-formating settings
end up in committing conflicting auto-formats?

Bruno

[1] https://gitlab.com/yade-dev/trunk/merge_requests/298#note_233001456



On Sun, 20 Oct 2019 at 21:47, Anton Gladky  wrote:

> Dear all,
>
> there are several pre-defined styles in the clang-formatter.
> I have recursively run all of them in the current trunk and I controlled,
> how many lines were changed (+ lines added, - lines removed)
>
>
> Style | +  | -   | Line length
> 
> LLVM  | 95796  | 71398   | 80
> Google| 95748  | 72468   | 80
> Chromium  | 104646 | 72689   | ?
> Mozilla   | 109952 | 70635   | 80
> WebKit| 76487  | 69974 * | ?
> Microsoft | 97828  | 73115   | ?
>
> As you can see the minimal diff was produced by the
> WebKit-style. It can be caused by the fact, that some
> styles are changing the line length of the code. So,
> this parameter is also in the table.
>
> After that I have took the WebKit style and played with
> the type of indents: spaces, tabs (Always and ForIndentation).
>
> UseTab  | IndentWidth  | +  | -
> 
> Never   | 1| 77654  | 71141
> Never   | 2| 73701  | 67188
> Never   | 4| 76487  | 69974
> Never   | 6| 78023  | 71510
> Never   | 8| 77785  | 71272
> ForIndentation  | 1| 77624  | 7
> ForIndentation  | 2| 73336  | 66823
> ForIndentation  | 4| 74727  | 68214
> ForIndentation  | 6| 77538  | 71025
> ForIndentation  | 8| 63022  | 56509 *
> Always  | 1| 77623  | 71110
> Always  | 2| 73340  | 66827
> Always  | 4| 74722  | 68209
> Always  | 8| 63013  | 56500
>
> I think that "tabs" won. Also "always" looks like too
> restrictive, so I have chosen this parameter with the
> indentwidth 8 as the minimal invasive.
>
> The last one is the maximal line length:
>
> ColumnLimit   | +  | -
> 
> 80| 99719  | 62788
> 100   | 83550  | 61566
> 120   | 75988  | 61015
> 140   | 71907  | 60655  *
> 160   | 69721  | 60506
> 180   | 68393  | 60430
> 200   | 67445  | 60374
> 220   | 66670  | 60316
> 240   | 66146  | 60304
>
> Results are interesting. Clear is that we need to
> cut over long lines. My proposal would be 140,
> though most of guidelines use 80.
>
> Opinions?
>
> Regards
>
> Anton
>
> Am Fr., 18. Okt. 2019 um 15:30 Uhr schrieb Janek Kozicki (yade)
> :
> >
> > It’s a great idea. And I’m surprised how personal it is for me :) Code
> readability is top priority for me. I’m sure we will find the clang-format
> settings which suits us.
> >
> > Automatic checks if the recently touched files (e.g files from the
> current MR) were reformatted will definitely help us. At first this will
> add some unreadable diffs. But later it will be a refreshing breeze :)
> >
> > -- Janek Kozicki
> > On 17 Oct 2019, 18:59 +0200, Anton Gladky ,
> wrote:
> >
> > Hi,
> >
> > I propose to use the clang-format [1], which automatically formats
> > the code according to the pre-defined rules. It has a nice support
> > by most of IDEs and can also be used through command line.
> >
> > There are some pre-defined styles (LLVM, Google, Chromium, Mozilla,
> WebKit),
> > and i think we just need to choose. Fine-tuning is also possible, but
> > it is not necessary.
> >
> > Running this tool recursively all over the code is possible, but can
> produce
> > a large, not reviewable diff. But formatting the files, touched during
> > development process can step-by-step fix most parts of the code and be
> > consistent in the future.
> >
> > I have prepared a WIP-merge request [2], where the .clang-format file is
> added
> > and the Scene.cpp was reformatted using this tool. So you can check,
> > whether it will work for us. Style format can be changed, but it looks
> like
> > LLVM-style will produce the minimal changes.
> >
> > Pipelines can also