[Bug build/30098] New: Keep trying clang-format

[Bug build/30098] New: Keep trying clang-format

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30098

            Bug ID: 30098
           Summary: Keep trying clang-format
           Product: gdb
           Version: HEAD
            Status: NEW
          Severity: normal
          Priority: P2
         Component: build
          Assignee: unassigned at sourceware dot org
          Reporter: tromey at sourceware dot org
  Target Milestone: ---

We periodically discuss clang-format.
I think we should have a bug to track our progress.

I'll attach my current .clang-format and add some commentary
about the problems.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/30098] Keep trying clang-format

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30098

--- Comment #1 from Tom Tromey <tromey at sourceware dot org> ---
Created attachment 14660
  --> https://sourceware.org/bugzilla/attachment.cgi?id=14660&action=edit
the clang-format config file

Here's my current .clang-format, updated to clang-format 14.

I updated by reading:

https://clang.llvm.org/docs/ClangFormatStyleOptions.html

and considering any parameter after 10 (the last time I updated)
and before 15 (my machine has 14).

Then to evaluate this I run it on various files and spot check them.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/30098] Keep trying clang-format

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30098

--- Comment #2 from Tom Tromey <tromey at sourceware dot org> ---
Last time I did this I noted a few upstream bugs:

Goto label indentation is wrong
https://github.com/llvm/llvm-project/issues/24492

Case label / goto label:
https://github.com/llvm/llvm-project/issues/53717

Bin-packing is a mess:
https://github.com/llvm/llvm-project/issues/37051
https://github.com/llvm/llvm-project/issues/31255

The first two of these aren't that important probably.
They result in minor differences but kind of meh ones really.

Bin-packing remains problematic for me.
I'll try to find an example.

There isn't an upstream bug open for this, but there's
no way to say that '_' is a special function not requiring
a space, so all the source is rewritten to '_ ("blah")'

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/30098] Keep trying clang-format

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30098

--- Comment #3 from Tom Tromey <tromey at sourceware dot org> ---
BTW there are a bunch of FIXMEs in the config file.
I don't really know how the penalty system works so
maybe that can be improved too.

Sometimes clang-format makes bad decisions.  One example
I'm looking at is if you have an 'if' that is a series of
clauses joined by '&&', sometimes it is clearest to put
each such clause on its own line.  However clang-format does this:

@@ -194,10 +193,8 @@ eq_bfd (const void *a, const void *b)
     = (const struct gdb_bfd_cache_search *) b;
   struct gdb_bfd_data *gdata = (struct gdb_bfd_data *) bfd_usrdata (abfd);

-  return (gdata->mtime == s->mtime
-         && gdata->size == s->size
-         && gdata->inode == s->inode
-         && gdata->device_id == s->device_id
+  return (gdata->mtime == s->mtime && gdata->size == s->size
+         && gdata->inode == s->inode && gdata->device_id == s->device_id
          && strcmp (bfd_get_filename (abfd), s->filename) == 0);
 }

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/30098] Keep trying clang-format

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30098

--- Comment #4 from Tom Tromey <tromey at sourceware dot org> ---
Also the config file needs a list of all the attribute
macros, e.g. from ansidecl.h (and elsewhere?).
Otherwise:

@@ -1148,11 +1124,11 @@ static bfd_error_handler_type
default_bfd_error_handler;
    per-inferior basis.  */

 static void ATTRIBUTE_PRINTF (1, 0)
-gdb_bfd_error_handler (const char *fmt, va_list ap)
+  gdb_bfd_error_handler (const char *fmt, va_list ap)
 {
   va_list ap_copy;

-  va_copy(ap_copy, ap);
+  va_copy (ap_copy, ap);
   const std::string str = string_vprintf (fmt, ap_copy);
   va_end (ap_copy);

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/30098] Keep trying clang-format

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30098

--- Comment #5 from Tom Tromey <tromey at sourceware dot org> ---
One final thought -- if we do ever run this on the repo,
it would be good to use some phony setting for --author,
so it's readily obvious in 'git annotate' that some commit
is just the reformatting.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/30098] Keep trying clang-format

[simark at simark dot ca]

https://sourceware.org/bugzilla/show_bug.cgi?id=30098

Simon Marchi <simark at simark dot ca> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |simark at simark dot ca

--- Comment #6 from Simon Marchi <simark at simark dot ca> ---
Thanks for filing this.

Can we make some branch on sourceware with the latest attempt at doing this,
that we could update over time?  I would suggest to have just two commits over
master, one commit to add .clang-format (and maybe a script to run clang-format
correctly to do a mass reformat) and another commit that is the actual mass
reformat.

Using clang-format-15, I see some changes that are not stable in
hppa-netbsd-tdep.c, clang-format changes the code every time I run it:

diff --git a/gdb/hppa-netbsd-tdep.c b/gdb/hppa-netbsd-tdep.c
index 5468a135caf7..7b4c8c71fe2f 100644
--- a/gdb/hppa-netbsd-tdep.c
+++ b/gdb/hppa-netbsd-tdep.c
@@ -63,30 +63,29 @@ static void hppanbsd_sigtramp_cache_init (const struct
tramp_frame *,
                                          struct trad_frame_cache *,
                                          CORE_ADDR);

-static const struct tramp_frame hppanbsd_sigtramp_si4 = {
-  SIGTRAMP_FRAME,
-  4,
-  { { 0xc7d7c012, ULONGEST_MAX }, /*   bb,>=,n %arg3, 30, 1f           */
-    { 0xd6e01c1e, ULONGEST_MAX }, /*    depwi 0,31,2,%arg3             */
-    { 0x0ee81093, ULONGEST_MAX }, /*   ldw 4(%arg3), %r19              */
-    { 0x0ee01097, ULONGEST_MAX }, /*   ldw 0(%arg3), %arg3             */
-    /* 1:                                      */
-    { 0xe8404000, ULONGEST_MAX }, /*   blr %r0, %rp                    */
-    { 0xeae0c002, ULONGEST_MAX }, /*   bv,n %r0(%arg3)                 */
-    { 0x08000240, ULONGEST_MAX }, /*    nop                            */
-
-    { 0x0803025a, ULONGEST_MAX }, /*   copy %r3, %arg0                 */
-    { 0x20200801, ULONGEST_MAX }, /*   ldil -40000000, %r1             */
-    { 0xe420e008, ULONGEST_MAX }, /*   be,l 4(%sr7, %r1), %sr0, %r31   */
-    { 0x34160268, ULONGEST_MAX }, /*    ldi 134, %t1 ; SYS_setcontext  */
-
-    { 0x081c025a, ULONGEST_MAX }, /*   copy ret0, %arg0                */
-    { 0x20200801, ULONGEST_MAX }, /*   ldil -40000000, %r1             */
-    { 0xe420e008, ULONGEST_MAX }, /*   be,l 4(%sr7, %r1), %sr0, %r31   */
-    { 0x34160002, ULONGEST_MAX }, /*    ldi 1, %t1 ; SYS_exit          */
-    { TRAMP_SENTINEL_INSN, ULONGEST_MAX } },
-  hppanbsd_sigtramp_cache_init
-};
+static const struct tramp_frame hppanbsd_sigtramp_si4
+  = { SIGTRAMP_FRAME,
+      4,
+      { { 0xc7d7c012, ULONGEST_MAX }, /*       bb,>=,n %arg3, 30, 1f          
*/
+       { 0xd6e01c1e, ULONGEST_MAX }, /*         depwi 0,31,2,%arg3            
*/
+       { 0x0ee81093, ULONGEST_MAX }, /*        ldw 4(%arg3), %r19             
*/
+       { 0x0ee01097, ULONGEST_MAX }, /*        ldw 0(%arg3), %arg3            
*/
+       /* 1:                                   */
+       { 0xe8404000, ULONGEST_MAX }, /*        blr %r0, %rp                   
*/
+       { 0xeae0c002, ULONGEST_MAX }, /*        bv,n %r0(%arg3)                
*/
+       { 0x08000240, ULONGEST_MAX }, /*         nop                           
*/
+
+       { 0x0803025a, ULONGEST_MAX }, /*        copy %r3, %arg0                
*/
+       { 0x20200801, ULONGEST_MAX }, /*        ldil -40000000, %r1            
*/
+       { 0xe420e008, ULONGEST_MAX }, /*        be,l 4(%sr7, %r1), %sr0, %r31  
*/
+       { 0x34160268, ULONGEST_MAX }, /*         ldi 134, %t1 ; SYS_setcontext 
*/
+
+       { 0x081c025a, ULONGEST_MAX }, /*        copy ret0, %arg0               
*/
+       { 0x20200801, ULONGEST_MAX }, /*        ldil -40000000, %r1            
*/
+       { 0xe420e008, ULONGEST_MAX }, /*        be,l 4(%sr7, %r1), %sr0, %r31  
*/
+       { 0x34160002, ULONGEST_MAX }, /*         ldi 1, %t1 ; SYS_exit         
*/
+       { TRAMP_SENTINEL_INSN, ULONGEST_MAX } },
+      hppanbsd_sigtramp_cache_init };

Probably a bug in clang-format?  I guess that could be worked around by placing
markers around that code to prevent clang-format from touching it.

In my .clang-format, I had `UseTab: Always`, but it doesn't seem to make a
difference compared to what you have.


> Sometimes clang-format makes bad decisions.  One example
> I'm looking at is if you have an 'if' that is a series of
> clauses joined by '&&', sometimes it is clearest to put
> each such clause on its own line.  However clang-format does this:
> 
> @@ -194,10 +193,8 @@ eq_bfd (const void *a, const void *b)
>      = (const struct gdb_bfd_cache_search *) b;
>    struct gdb_bfd_data *gdata = (struct gdb_bfd_data *) bfd_usrdata (abfd);
>  
> -  return (gdata->mtime == s->mtime
> -	  && gdata->size == s->size
> -	  && gdata->inode == s->inode
> -	  && gdata->device_id == s->device_id
> +  return (gdata->mtime == s->mtime && gdata->size == s->size
> +	  && gdata->inode == s->inode && gdata->device_id == s->device_id
>  	  && strcmp (bfd_get_filename (abfd), s->filename) == 0);
>  }

I agree that the old one is more readable.  I suppose that could be a
clang-format rule, similar to BinPackParameters.  If an expression fits on one
line, leave it on one line.  Otherwise, break at binary operators.  It could
happen with other operators too, like:

 something = (aaaaaaaa + bbbbbbbb + cccccccc
              + dddddddd); 

vs

 something = (aaaaaaaa
              + bbbbbbbb
              + cccccccc
              + dddddddd);

Of course, humans will pretty much always make better decisions for things like
this.  For instance, you might want to have it organized logically like this:

 path = (base
         + "/" + subdir1
         + "/" + subdir2
         + "/" + subdir3);

But the tool will either choose:


 path = (base + "/" + subdir1 + "/"
         + subdir2 + "/" + subdir3);

of

 path = (base
         + "/"
         + subdir1
         + "/"
         + subdir2
         + "/"
         + subdir3);

If we want to use an automated tool, we have to accept the tradeoff that some
cases will be less nice, in exchange for convenience.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/30098] Keep trying clang-format

[jan at vrany dot io]

https://sourceware.org/bugzilla/show_bug.cgi?id=30098

Jan Vrany <jan at vrany dot io> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |jan at vrany dot io

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/30098] Keep trying clang-format

[sam at gentoo dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30098

Sam James <sam at gentoo dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |sam at gentoo dot org

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/30098] Keep trying clang-format

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30098

--- Comment #7 from Tom Tromey <tromey at sourceware dot org> ---
> Probably a bug in clang-format?

Seems like it has to be, formatters ought to be idempotent.
Otherwise if we use it we will get a lot of commit churn.

> If we want to use an automated tool, we have to accept the tradeoff that some cases will be less nice, in exchange for convenience.

Yeah, I agree.  I just feel some things are too ugly.
I can probably live with most of the changes clang-format imposes,
but I am not sure that is true for all of them.

Also I found out that it doesn't format lambdas close to what we like.
It puts the opening brace at the wrong spot, so I wonder if maybe
we're missing a config item.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug build/30098] Keep trying clang-format

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30098

--- Comment #8 from Tom Tromey <tromey at sourceware dot org> ---
FWIW I looked at the new settings for clang-format 15 and 16
and they didn't seem useful to gdb.
Also none of the bugs linked above seem to have been fixed.

-- 
You are receiving this mail because:
You are on the CC list for the bug.