Re: Some points on clang-format usage

[Miklos Vajna]

Hi Stephan,

On Mon, Nov 04, 2019 at 11:08:43AM +0100, Stephan Bergmann 
 wrote:
> And another point that just occurred to me is that we will probably run into
> problems at some point in the future:  It is likely that future C++
> standards will introduce constructs that cannot be parsed by clang-format
> 5.0.0, and that LO will use such constructs in files that are processed by
> clang-format.

I'm happy to investigate an upgrade if there is a practical need for it.
I'm not sure we have any of that today; perhaps structured bindings?

But limiting the number of upgrades (to avoid reformat noise) probably
has some value.

Regards,

Miklos


signature.asc
Description: Digital signature
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Some points on clang-format usage

[Stephan Bergmann]

On 17/11/2017 10:03, Stephan Bergmann wrote:

Some random points regarding our recently introduced use of clang-format:


And another point that just occurred to me is that we will probably run 
into problems at some point in the future:  It is likely that future C++ 
standards will introduce constructs that cannot be parsed by 
clang-format 5.0.0, and that LO will use such constructs in files that 
are processed by clang-format.


Just noting that down for now.

___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Some points on clang-format usage

[Chris Sherlock]

On 12 Mar 2018, at 7:38 pm, Miklos Vajna  wrote:
> 
> Hi Chris,
> 
> On Sun, Mar 11, 2018 at 05:28:40PM +1100, Chris Sherlock 
>  wrote:
>> Is there a wiki page on this
> 
> solenv/clang-format/README documents this:
> 
>> - blacklist: list of existing files not to be formatted:
>> 
>>  - if you rename a file mentioned in this list, please update the entry (and
>>   keep the file sorted)
> 
> I hope that's enough and we don't need a dedicated wiki page for this. :-)
> 
> Regards,
> 
> Miklos
> 

Ta, I definitely overlooked that. 

Sorry for the knee jerk reaction. 

Thanks,
Chris
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Some points on clang-format usage

[Stephan Bergmann]

On 11/03/18 07:44, Chris Sherlock wrote:

And a quick comment - back in the day I split OutputDevice into seperate files. 
People were actually very supportive of this change. You asked me why I split 
out the files, but bitmap functions are already split. So how is it useful? 
Well, we could combine all the functions into one file, because bitmap3.cxx as 
a filename is less than useful.


I asked for a rationale, yes.  To me at least, that change 
() looked rather random.  And 
combined with the fact that it cosmetically changed the moved code, that 
made me a bit nervous (because that makes tracking the code's history 
unnecessarily harder).



I have been consulting with Tomasz about my code changes, by the way. He has 
been working on the bitmap code, and I’ve been writing bitmap tests, which 
apparently are useful. If you don’t like the unit tests, please let me know - 
after all, I’ve stoped looking at the SAL as it got people’s noses out of 
joint. Given I’ve previously worked on the VCL and even fixed bugs, I thought 
that would be alright.


Why do you think that I might not like certain unit tests?


Stephan, are you actively working on vcl bitmap code? I am happy to add you as 
a reviewer to my gerrit commits, would you like me to do this in future? Are 
there any other modules you want me to add you as a reviewer?


No, I'm not actively working on vcl bitmap code, and I have little 
insight into that code anyway.  However, what I do care about is that if 
I have to look into that code, any code, that it isn't unnecessarily 
hard to understand what changes have been made to the code over time, 
for which reasons.



On 6 Mar 2018, at 8:25 pm, Stephan Bergmann  wrote:

On 17.11.2017 10:03, Stephan Bergmann wrote:
* Don't reformat when moving an existing file
When moving an old file that is listed in solenv/clang-format/blacklist (so 
doesn't automatically get reformatted upon commit), adapt 
solenv/clang-format/blacklist to contain the new name, instead of provoking a 
reformat upon commit.  Otherwise, if the reformatted code looks sufficiently 
different from the original, it's hard to impossible to keep track in the git 
history of which file was moved where.


Please also follow this advice when moving part of an existing file out to a new file, as 
happened e.g. at 

 "vcl: split painting bitmap functions to bitmappaint.cxx".

___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Some points on clang-format usage

[Miklos Vajna]

Hi Chris,

On Sun, Mar 11, 2018 at 05:28:40PM +1100, Chris Sherlock 
 wrote:
> Is there a wiki page on this

solenv/clang-format/README documents this:

> - blacklist: list of existing files not to be formatted:
> 
>   - if you rename a file mentioned in this list, please update the entry (and
>keep the file sorted)

I hope that's enough and we don't need a dedicated wiki page for this. :-)

Regards,

Miklos


signature.asc
Description: Digital signature
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Some points on clang-format usage

[Chris Sherlock]

And a quick comment - back in the day I split OutputDevice into seperate files. 
People were actually very supportive of this change. You asked me why I split 
out the files, but bitmap functions are already split. So how is it useful? 
Well, we could combine all the functions into one file, because bitmap3.cxx as 
a filename is less than useful.

I have been consulting with Tomasz about my code changes, by the way. He has 
been working on the bitmap code, and I’ve been writing bitmap tests, which 
apparently are useful. If you don’t like the unit tests, please let me know - 
after all, I’ve stoped looking at the SAL as it got people’s noses out of 
joint. Given I’ve previously worked on the VCL and even fixed bugs, I thought 
that would be alright.

I have some other changes I’ve suggested, I’m going to submit them to gerrit 
when I get back from visiting my brother in Victoria.

Stephan, are you actively working on vcl bitmap code? I am happy to add you as 
a reviewer to my gerrit commits, would you like me to do this in future? Are 
there any other modules you want me to add you as a reviewer?

Chris

Sent from my iPad

> On 6 Mar 2018, at 8:25 pm, Stephan Bergmann  wrote:
> 
>> On 17.11.2017 10:03, Stephan Bergmann wrote:
>> * Don't reformat when moving an existing file
>> When moving an old file that is listed in solenv/clang-format/blacklist (so 
>> doesn't automatically get reformatted upon commit), adapt 
>> solenv/clang-format/blacklist to contain the new name, instead of provoking 
>> a reformat upon commit.  Otherwise, if the reformatted code looks 
>> sufficiently different from the original, it's hard to impossible to keep 
>> track in the git history of which file was moved where.
> 
> Please also follow this advice when moving part of an existing file out to a 
> new file, as happened e.g. at 
> 
>  "vcl: split painting bitmap functions to bitmappaint.cxx".
> ___
> LibreOffice mailing list
> LibreOffice@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/libreoffice
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Some points on clang-format usage

[Chris Sherlock]

I wasn’t aware of this.

Is there a wiki page on this, or is this one of those things where you must be 
closely following the dev mailing list? 

Again, another instance where a convention I wasn’t aware of has got me caught 
out. Rather feels like I’m being picked on, as per normal. 

Chris

Sent from my iPad

> On 6 Mar 2018, at 8:25 pm, Stephan Bergmann  wrote:
> 
>> On 17.11.2017 10:03, Stephan Bergmann wrote:
>> * Don't reformat when moving an existing file
>> When moving an old file that is listed in solenv/clang-format/blacklist (so 
>> doesn't automatically get reformatted upon commit), adapt 
>> solenv/clang-format/blacklist to contain the new name, instead of provoking 
>> a reformat upon commit.  Otherwise, if the reformatted code looks 
>> sufficiently different from the original, it's hard to impossible to keep 
>> track in the git history of which file was moved where.
> 
> Please also follow this advice when moving part of an existing file out to a 
> new file, as happened e.g. at 
> 
>  "vcl: split painting bitmap functions to bitmappaint.cxx".
> ___
> LibreOffice mailing list
> LibreOffice@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/libreoffice
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Some points on clang-format usage

[Stephan Bergmann]

On 17.11.2017 10:03, Stephan Bergmann wrote:

* Don't reformat when moving an existing file

When moving an old file that is listed in solenv/clang-format/blacklist 
(so doesn't automatically get reformatted upon commit), adapt 
solenv/clang-format/blacklist to contain the new name, instead of 
provoking a reformat upon commit.  Otherwise, if the reformatted code 
looks sufficiently different from the original, it's hard to impossible 
to keep track in the git history of which file was moved where.


Please also follow this advice when moving part of an existing file out 
to a new file, as happened e.g. at 
 
"vcl: split painting bitmap functions to bitmappaint.cxx".

___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Some points on clang-format usage

[Miklos Vajna]

Hi,

On Thu, Dec 14, 2017 at 11:28:57AM +0100, Stephan Bergmann 
 wrote:
> I'm somewhat undecided whether or not to do that change to .clang-format
> now.  My feeling is that reformatting of comments is a pain and a gotcha
> anyway.

Turning this on would also have the effect that e.g.

> void f() {
> statement1(); // lengthy comment comment comment comment comment comment 
> comment comment comment
>  // comment comment comment comment comment comment
> statement2();
> }

would be formatted (due to the extra -- probably unwanted -- space) as

> void f() {
> statement1(); // lengthy comment comment comment comment comment comment 
> comment comment comment
>   // comment comment comment comment comment comment
> statement2();
> }

which might be misleading as well. So leaving at as-is has some
benefits, I think.

Regards,

Miklos


signature.asc
Description: Digital signature
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Some points on clang-format usage

[Stephan Bergmann]

On 12/14/2017 11:08 AM, Stephan Bergmann wrote:

On 12/14/2017 10:35 AM, Miklos Vajna wrote:

[...]

This is due to our explicit AlignTrailingComments=false, it may make
sense to consider enabling it (if
solenv/clang-format/reformat-formatted-files confirms this solves more
problems than it causes).

[...]

Namely the inconsistency in reformatting from

void decl1(); // lengthy comment comment comment comment comment 
comment comment comment
  // comment comment comment comment comment comment 
comment pertaining to decl1
void decl2() {} // lengthy comment comment comment comment comment 
comment comment comment
    // comment comment comment comment comment comment 
comment pertaining to decl2

void decl3();


to

void decl1(); // lengthy comment comment comment comment comment 
comment comment comment
    // comment comment comment comment comment comment comment 
pertaining to decl1
void decl2() {} // lengthy comment comment comment comment comment 
comment comment comment
// comment comment comment comment comment comment comment pertaining 
to decl2

void decl3();


treating comments following function definitions differently than those 
following mere function declarations.


It appears that changing AlignTrailingCommentst to true would solve 
that, and would reformat from



void decl1(); // lengthy comment comment comment comment comment comment 
comment comment
  // comment comment comment comment comment comment comment 
pertaining to decl1
void decl2() {} // lengthy comment comment comment comment comment comment 
comment comment
// comment comment comment comment comment comment comment 
pertaining to decl2
// Comment pertaining to decl3:
void decl3();


to


void decl1();   // lengthy comment comment comment comment comment comment 
comment comment
// comment comment comment comment comment comment comment 
pertaining to decl1
void decl2() {} // lengthy comment comment comment comment comment comment 
comment comment
// comment comment comment comment comment comment comment 
pertaining to decl2
// Comment pertaining to decl3:
void decl3();


(and the changes it would cause to 
`solenv/clang-format/reformat-formatted-files` would appear to be OK, too).


I'm somewhat undecided whether or not to do that change to .clang-format 
now.  My feeling is that reformatting of comments is a pain and a gotcha 
anyway.

___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Some points on clang-format usage

[Stephan Bergmann]

On 12/14/2017 10:35 AM, Miklos Vajna wrote:

On Wed, Dec 13, 2017 at 10:50:31AM +0100, Stephan Bergmann 
 wrote:

Another gotcha to watch out for related to comments is how


void decl1(); // lengthy comment comment comment comment comment comment 
comment comment comment
   // comment comment comment comment comment comment comment 
comment pertaining to decl1
void decl2();
 // lengthy comment comment comment comment comment comment comment comment 
pertaining to decl2
void decl3();

void f() {
 statement1(); // lengthy comment comment comment comment comment comment 
comment comment comment
   // comment comment comment comment comment comment 
pertaining to statement1
 statement2();
 // lengthy comment comment comment comment comment comment comment 
pertaining to statement2
 statement3();
}


is reformatted to


void decl1(); // lengthy comment comment comment comment comment comment 
comment comment comment
 // comment comment comment comment comment comment comment comment 
pertaining to decl1


This is due to our explicit AlignTrailingComments=false, it may make
sense to consider enabling it (if
solenv/clang-format/reformat-formatted-files confirms this solves more
problems than it causes).


void decl2();
// lengthy comment comment comment comment comment comment comment comment 
pertaining to decl2
void decl3();


I think this style was discussed on IRC already, and there was no
consensus if even the original form is misleading to the reader of the
code or not (so I did not research so far if there is an option to
preserve that style).


I now find that my examples above didn't even show what actually pissed 
me off when I tried to get a comment past clang-format in 
 
"Fix isSalCallFunction so it also works on Windows".


Namely the inconsistency in reformatting from


void decl1(); // lengthy comment comment comment comment comment comment 
comment comment
  // comment comment comment comment comment comment comment 
pertaining to decl1
void decl2() {} // lengthy comment comment comment comment comment comment 
comment comment
// comment comment comment comment comment comment comment 
pertaining to decl2
void decl3();


to


void decl1(); // lengthy comment comment comment comment comment comment 
comment comment
// comment comment comment comment comment comment comment pertaining to 
decl1
void decl2() {} // lengthy comment comment comment comment comment comment 
comment comment
// comment comment comment comment comment comment comment pertaining to decl2
void decl3();


treating comments following function definitions differently than those 
following mere function declarations.

___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Some points on clang-format usage

[Miklos Vajna]

Hi,

On Wed, Dec 13, 2017 at 10:50:31AM +0100, Stephan Bergmann 
 wrote:
> Another gotcha to watch out for related to comments is how
> 
> > void decl1(); // lengthy comment comment comment comment comment comment 
> > comment comment comment
> >   // comment comment comment comment comment comment comment 
> > comment pertaining to decl1
> > void decl2();
> > // lengthy comment comment comment comment comment comment comment 
> > comment pertaining to decl2
> > void decl3();
> > 
> > void f() {
> > statement1(); // lengthy comment comment comment comment comment 
> > comment comment comment comment
> >   // comment comment comment comment comment comment 
> > pertaining to statement1
> > statement2();
> > // lengthy comment comment comment comment comment comment comment 
> > pertaining to statement2
> > statement3();
> > }
> 
> is reformatted to
> 
> > void decl1(); // lengthy comment comment comment comment comment comment 
> > comment comment comment
> > // comment comment comment comment comment comment comment comment 
> > pertaining to decl1

This is due to our explicit AlignTrailingComments=false, it may make
sense to consider enabling it (if
solenv/clang-format/reformat-formatted-files confirms this solves more
problems than it causes).

> > void decl2();
> > // lengthy comment comment comment comment comment comment comment comment 
> > pertaining to decl2
> > void decl3();

I think this style was discussed on IRC already, and there was no
consensus if even the original form is misleading to the reader of the
code or not (so I did not research so far if there is an option to
preserve that style).

Regards,

Miklos


signature.asc
Description: Digital signature
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Some points on clang-format usage

[Stephan Bergmann]

On 11/17/2017 10:03 AM, Stephan Bergmann wrote:

* clang-format cannot automatically reformat code

Most comments in the code are written by humans, for humans.  They are 
often placed in a way that makes it technically unclear what code they 
pertain to, relying on humans to nevertheless easily discern that.  For 
example, in


void f(int x) { g(x + 1); } // long comment explaining how g 
internally subtracts one


the comment clearly pertains to the call to g.  Yet, clang-format will 
reformat this to something like



void f(int x)
{
    g(x + 1);
} // long comment explaining how g internally subtracts one


instead of


void f(int x)
{
    g(x + 1); // long comment explaining how g internally subtracts one
}


thereby obscuring things.  Always review automatically reformatted code 
for glitches like this.



Another gotcha to watch out for related to comments is how


void decl1(); // lengthy comment comment comment comment comment comment 
comment comment comment
  // comment comment comment comment comment comment comment 
comment pertaining to decl1
void decl2();
// lengthy comment comment comment comment comment comment comment comment 
pertaining to decl2
void decl3();

void f() {
statement1(); // lengthy comment comment comment comment comment comment 
comment comment comment
  // comment comment comment comment comment comment pertaining 
to statement1
statement2();
// lengthy comment comment comment comment comment comment comment 
pertaining to statement2
statement3();
}


is reformatted to


void decl1(); // lengthy comment comment comment comment comment comment 
comment comment comment
// comment comment comment comment comment comment comment comment 
pertaining to decl1
void decl2();
// lengthy comment comment comment comment comment comment comment comment 
pertaining to decl2
void decl3();

void f()
{
statement1(); // lengthy comment comment comment comment comment comment 
comment comment comment
// comment comment comment comment comment comment pertaining to 
statement1
statement2();
// lengthy comment comment comment comment comment comment comment 
pertaining to statement2
statement3();
}

___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Some points on clang-format usage

[Stephan Bergmann]

On 11/17/2017 12:19 PM, Miklos Vajna wrote:

On Fri, Nov 17, 2017 at 11:16:13AM +0100, Stephan Bergmann 
 wrote:

...which might be reason enough to still revisit the .clang-format rule that
causes leading commas, if that wrecks such havoc with comments?


I think BreakConstructorInitializersBeforeComma is the relevant key.
Setting it to false would not be controversial, it would just mean we
leave that area alone, I believe.


So among 
BreakConstructorInitializers:{BeforeColon,BeforeComma,AfterColon} and 
ConstructorInitializerAllOnOneLineOrOnePerLine:{false,true} there 
appears to be no setting that both (a) consistently places each 
initializer on a line of its own (the current behaviour IIUC; beneficial 
for small diffs when modifying an initializer list) and (b) puts the 
comma after the initializer (which would be beneficial for preserving 
trailing comment placement).


Shrug.
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Some points on clang-format usage

[Miklos Vajna]

Hi Stephan,

On Fri, Nov 17, 2017 at 11:16:13AM +0100, Stephan Bergmann 
 wrote:
> ...which might be reason enough to still revisit the .clang-format rule that
> causes leading commas, if that wrecks such havoc with comments?

I think BreakConstructorInitializersBeforeComma is the relevant key.
Setting it to false would not be controversial, it would just mean we
leave that area alone, I believe.

Regards,

Miklos


signature.asc
Description: Digital signature
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Some points on clang-format usage

[Stephan Bergmann]

On 11/17/2017 10:54 AM, Miklos Vajna wrote:

Thanks for mentioning this. I also noticed something similar with
initializer lists, where

Foo::Foo()
 : foo(1), // foo
   bar(2) // bar
{
}

gets reformatted to:

Foo::Foo()
 : foo(1)
 , // foo
 bar(2) // bar
{
}

but the ideal form is probably

Foo::Foo()
 : foo(1) // foo
 , bar(2) // bar
{
}

instead.


...which might be reason enough to still revisit the .clang-format rule 
that causes leading commas, if that wrecks such havoc with comments?

___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Re: Some points on clang-format usage

[Miklos Vajna]

Hi Stephan,

On Fri, Nov 17, 2017 at 10:03:11AM +0100, Stephan Bergmann 
 wrote:
> Some random points regarding our recently introduced use of clang-format:
> 
> 
> * clang-format cannot automatically reformat code
> 
> Most comments in the code are written by humans, for humans.  They are often
> placed in a way that makes it technically unclear what code they pertain to,
> relying on humans to nevertheless easily discern that.  For example, in
> 
> > void f(int x) { g(x + 1); } // long comment explaining how g internally 
> > subtracts one
> 
> the comment clearly pertains to the call to g.  Yet, clang-format will
> reformat this to something like
> 
> > void f(int x)
> > {
> > g(x + 1);
> > } // long comment explaining how g internally subtracts one
> 
> instead of
> 
> > void f(int x)
> > {
> > g(x + 1); // long comment explaining how g internally subtracts one
> > }
> 
> thereby obscuring things.  Always review automatically reformatted code for
> glitches like this.

Thanks for mentioning this. I also noticed something similar with
initializer lists, where

Foo::Foo()
: foo(1), // foo
  bar(2) // bar
{
}

gets reformatted to:

Foo::Foo()
: foo(1)
, // foo
bar(2) // bar
{
}

but the ideal form is probably

Foo::Foo()
: foo(1) // foo
, bar(2) // bar
{
}

instead.

Regards,

Miklos


signature.asc
Description: Digital signature
___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice

Some points on clang-format usage

[Stephan Bergmann]

Some random points regarding our recently introduced use of clang-format:


* clang-format cannot automatically reformat code

Most comments in the code are written by humans, for humans.  They are 
often placed in a way that makes it technically unclear what code they 
pertain to, relying on humans to nevertheless easily discern that.  For 
example, in



void f(int x) { g(x + 1); } // long comment explaining how g internally 
subtracts one


the comment clearly pertains to the call to g.  Yet, clang-format will 
reformat this to something like



void f(int x)
{
g(x + 1);
} // long comment explaining how g internally subtracts one


instead of


void f(int x)
{
g(x + 1); // long comment explaining how g internally subtracts one
}


thereby obscuring things.  Always review automatically reformatted code 
for glitches like this.



* Don't reformat when moving an existing file

When moving an old file that is listed in solenv/clang-format/blacklist 
(so doesn't automatically get reformatted upon commit), adapt 
solenv/clang-format/blacklist to contain the new name, instead of 
provoking a reformat upon commit.  Otherwise, if the reformatted code 
looks sufficiently different from the original, it's hard to impossible 
to keep track in the git history of which file was moved where.



* Keep your local .clang-format up to date

There's been changes to it, and there might be more coming (though 
that's hopefully getting less and less likely).  If you create commits 
involving new files on an old master revision, you risk wrongly 
formatted code (and thus the need for reformatting follow-up commits) 
when that commit is eventually cherry-picked into the upstream git repo. 
 "Pull early, pull often."

___
LibreOffice mailing list
LibreOffice@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/libreoffice