LinuxLists.cc - [PATCH] scsi: megaraid: cleanup formatting of megaraid

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Thu, 27 Jan 2022, [email protected] wrote:

> From: Tom Rix <[email protected]>
>
> checkpatch reports several hundred formatting errors. Run these files
> through clang-format and knock off some of them.
>

That method seems like a good recipe for endless churn unless checkpatch
and clang-format really agree about these style rules.

Why use checkpatch to assess code style, if we could simply diff the
existing source with the output from clang-format... but it seems that
clang-format harms readability, makes indentation errors and uses
inconsistent style rules. Some examples:

> static unsigned short int max_sectors_per_io = MAX_SECTORS_PER_IO;
> module_param(max_sectors_per_io, ushort, 0);
> -MODULE_PARM_DESC(max_sectors_per_io, "Maximum number of sectors per I/O request (default=MAX_SECTORS_PER_IO=128)");
> -
> +MODULE_PARM_DESC(
> + max_sectors_per_io,
> + "Maximum number of sectors per I/O request (default=MAX_SECTORS_PER_IO=128)");
>
> static unsigned short int max_mbox_busy_wait = MBOX_BUSY_WAIT;
> module_param(max_mbox_busy_wait, ushort, 0);
> -MODULE_PARM_DESC(max_mbox_busy_wait, "Maximum wait for mailbox in microseconds if busy (default=MBOX_BUSY_WAIT=10)");
> +MODULE_PARM_DESC(
> + max_mbox_busy_wait,
> + "Maximum wait for mailbox in microseconds if busy (default=MBOX_BUSY_WAIT=10)");
>

This code is longer for no real improvement.

>
> /*
> * The File Operations structure for the serial/ioctl interface of the driver
> */
> static const struct file_operations megadev_fops = {
> - .owner = THIS_MODULE,
> - .unlocked_ioctl = megadev_unlocked_ioctl,
> - .open = megadev_open,
> - .llseek = noop_llseek,
> + .owner = THIS_MODULE,
> + .unlocked_ioctl = megadev_unlocked_ioctl,
> + .open = megadev_open,
> + .llseek = noop_llseek,
> };
>
> /*

Readability loss.

> - prod_info_dma_handle = dma_map_single(&adapter->dev->dev,
> - (void *)&adapter->product_info,
> - sizeof(mega_product_info),
> - DMA_FROM_DEVICE);
> + prod_info_dma_handle = dma_map_single(
> + &adapter->dev->dev, (void *)&adapter->product_info,
> + sizeof(mega_product_info), DMA_FROM_DEVICE);
>

Note the orphaned first parameter and odd indentation.

>
> static DEF_SCSI_QCMD(megaraid_queue)
>
> -/**
> + /**
> * mega_allocate_scb()
> * @adapter: pointer to our soft state
> * @cmd: scsi command from the mid-layer

Indentation error.

> @@ -418,15 +409,14 @@ static DEF_SCSI_QCMD(megaraid_queue)
> * Allocate a SCB structure. This is the central structure for controller
> * commands.
> */
> -static inline scb_t *
> -mega_allocate_scb(adapter_t *adapter, struct scsi_cmnd *cmd)
> + static inline scb_t *mega_allocate_scb(adapter_t *adapter,
> + struct scsi_cmnd *cmd)
> {
> struct list_head *head = &adapter->free_list;

Same.

> @@ -586,26 +568,25 @@ mega_build_cmd(adapter_t *adapter, struct scsi_cmnd *cmd, int *busy)
>
> ldrv_num = mega_get_ldrv_num(adapter, cmd, channel);
>
> -
> max_ldrv_num = (adapter->flag & BOARD_40LD) ?
> - MAX_LOGICAL_DRIVES_40LD : MAX_LOGICAL_DRIVES_8LD;
> + MAX_LOGICAL_DRIVES_40LD :
> + MAX_LOGICAL_DRIVES_8LD;
>

Churn, if not readability loss. Note the indentation change here is
inconsistent with the indentation change noted above.

> * 6-byte READ(0x08) or WRITE(0x0A) cdb
> */
> - if( cmd->cmd_len == 6 ) {
> - mbox->m_out.numsectors = (u32) cmd->cmnd[4];
> - mbox->m_out.lba =
> - ((u32)cmd->cmnd[1] << 16) |
> - ((u32)cmd->cmnd[2] << 8) |
> - (u32)cmd->cmnd[3];
> + if (cmd->cmd_len == 6) {
> + mbox->m_out.numsectors = (u32)cmd->cmnd[4];
> + mbox->m_out.lba = ((u32)cmd->cmnd[1] << 16) |
> + ((u32)cmd->cmnd[2] << 8) |
> + (u32)cmd->cmnd[3];
>
> mbox->m_out.lba &= 0x1FFFFF;
>

Here, the orphaned term is moved up, next to the =. And yet,

>
> /* Calculate Scatter-Gather info */
> - mbox->m_out.numsgelements = mega_build_sglist(adapter, scb,
> - (u32 *)&mbox->m_out.xferaddr, &seg);
> + mbox->m_out.numsgelements =
> + mega_build_sglist(adapter, scb,
> + (u32 *)&mbox->m_out.xferaddr,
> + &seg);
>
> return scb;
>

... here the first term is moved down and orphaned, which is another
inconsistency.

2022-01-28 20:41:50

by Nick Desaulniers

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

+ Miguel (the clang-format maintainer), Joe (checkpatch maintainer)
These criticisms are worth reviewing.

On Thu, Jan 27, 2022 at 2:38 PM Finn Thain <[email protected]> wrote:
>
>
> On Thu, 27 Jan 2022, [email protected] wrote:
>
> > From: Tom Rix <[email protected]>
> >
> > checkpatch reports several hundred formatting errors. Run these files
> > through clang-format and knock off some of them.
> >
>
> That method seems like a good recipe for endless churn unless checkpatch
> and clang-format really agree about these style rules.
>
> Why use checkpatch to assess code style, if we could simply diff the
> existing source with the output from clang-format... but it seems that
> clang-format harms readability, makes indentation errors and uses
> inconsistent style rules. Some examples:
>
> > static unsigned short int max_sectors_per_io = MAX_SECTORS_PER_IO;
> > module_param(max_sectors_per_io, ushort, 0);
> > -MODULE_PARM_DESC(max_sectors_per_io, "Maximum number of sectors per I/O request (default=MAX_SECTORS_PER_IO=128)");
> > -
> > +MODULE_PARM_DESC(
> > + max_sectors_per_io,
> > + "Maximum number of sectors per I/O request (default=MAX_SECTORS_PER_IO=128)");
> >
> > static unsigned short int max_mbox_busy_wait = MBOX_BUSY_WAIT;
> > module_param(max_mbox_busy_wait, ushort, 0);
> > -MODULE_PARM_DESC(max_mbox_busy_wait, "Maximum wait for mailbox in microseconds if busy (default=MBOX_BUSY_WAIT=10)");
> > +MODULE_PARM_DESC(
> > + max_mbox_busy_wait,
> > + "Maximum wait for mailbox in microseconds if busy (default=MBOX_BUSY_WAIT=10)");
> >
>
> This code is longer for no real improvement.
>
> >
> > /*
> > * The File Operations structure for the serial/ioctl interface of the driver
> > */
> > static const struct file_operations megadev_fops = {
> > - .owner = THIS_MODULE,
> > - .unlocked_ioctl = megadev_unlocked_ioctl,
> > - .open = megadev_open,
> > - .llseek = noop_llseek,
> > + .owner = THIS_MODULE,
> > + .unlocked_ioctl = megadev_unlocked_ioctl,
> > + .open = megadev_open,
> > + .llseek = noop_llseek,
> > };
> >
> > /*
>
> Readability loss.
>
> > - prod_info_dma_handle = dma_map_single(&adapter->dev->dev,
> > - (void *)&adapter->product_info,
> > - sizeof(mega_product_info),
> > - DMA_FROM_DEVICE);
> > + prod_info_dma_handle = dma_map_single(
> > + &adapter->dev->dev, (void *)&adapter->product_info,
> > + sizeof(mega_product_info), DMA_FROM_DEVICE);
> >
>
> Note the orphaned first parameter and odd indentation.
>
> >
> > static DEF_SCSI_QCMD(megaraid_queue)
> >
> > -/**
> > + /**
> > * mega_allocate_scb()
> > * @adapter: pointer to our soft state
> > * @cmd: scsi command from the mid-layer
>
> Indentation error.
>
> > @@ -418,15 +409,14 @@ static DEF_SCSI_QCMD(megaraid_queue)
> > * Allocate a SCB structure. This is the central structure for controller
> > * commands.
> > */
> > -static inline scb_t *
> > -mega_allocate_scb(adapter_t *adapter, struct scsi_cmnd *cmd)
> > + static inline scb_t *mega_allocate_scb(adapter_t *adapter,
> > + struct scsi_cmnd *cmd)
> > {
> > struct list_head *head = &adapter->free_list;
>
> Same.
>
> > @@ -586,26 +568,25 @@ mega_build_cmd(adapter_t *adapter, struct scsi_cmnd *cmd, int *busy)
> >
> > ldrv_num = mega_get_ldrv_num(adapter, cmd, channel);
> >
> > -
> > max_ldrv_num = (adapter->flag & BOARD_40LD) ?
> > - MAX_LOGICAL_DRIVES_40LD : MAX_LOGICAL_DRIVES_8LD;
> > + MAX_LOGICAL_DRIVES_40LD :
> > + MAX_LOGICAL_DRIVES_8LD;
> >
>
> Churn, if not readability loss. Note the indentation change here is
> inconsistent with the indentation change noted above.
>
> > * 6-byte READ(0x08) or WRITE(0x0A) cdb
> > */
> > - if( cmd->cmd_len == 6 ) {
> > - mbox->m_out.numsectors = (u32) cmd->cmnd[4];
> > - mbox->m_out.lba =
> > - ((u32)cmd->cmnd[1] << 16) |
> > - ((u32)cmd->cmnd[2] << 8) |
> > - (u32)cmd->cmnd[3];
> > + if (cmd->cmd_len == 6) {
> > + mbox->m_out.numsectors = (u32)cmd->cmnd[4];
> > + mbox->m_out.lba = ((u32)cmd->cmnd[1] << 16) |
> > + ((u32)cmd->cmnd[2] << 8) |
> > + (u32)cmd->cmnd[3];
> >
> > mbox->m_out.lba &= 0x1FFFFF;
> >
>
> Here, the orphaned term is moved up, next to the =. And yet,
>
> >
> > /* Calculate Scatter-Gather info */
> > - mbox->m_out.numsgelements = mega_build_sglist(adapter, scb,
> > - (u32 *)&mbox->m_out.xferaddr, &seg);
> > + mbox->m_out.numsgelements =
> > + mega_build_sglist(adapter, scb,
> > + (u32 *)&mbox->m_out.xferaddr,
> > + &seg);
> >
> > return scb;
> >
>
> ... here the first term is moved down and orphaned, which is another
> inconsistency.

--
Thanks,
~Nick Desaulniers

2022-01-29 00:26:45

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On 1/27/22 2:47 PM, Nick Desaulniers wrote:
> + Miguel (the clang-format maintainer), Joe (checkpatch maintainer)
> These criticisms are worth reviewing.
>
> On Thu, Jan 27, 2022 at 2:38 PM Finn Thain <[email protected]> wrote:
>>
>> On Thu, 27 Jan 2022, [email protected] wrote:
>>
>>> From: Tom Rix <[email protected]>
>>>
>>> checkpatch reports several hundred formatting errors. Run these files
>>> through clang-format and knock off some of them.
>>>
>> That method seems like a good recipe for endless churn unless checkpatch
>> and clang-format really agree about these style rules.
>>
>> Why use checkpatch to assess code style, if we could simply diff the
>> existing source with the output from clang-format... but it seems that
>> clang-format harms readability, makes indentation errors and uses
>> inconsistent style rules. Some examples:

Problems with clang-format should be fixed, I'll take a look.

I was reviewing this file for another issue and could not get past how
horredously bad it was and really did not want to manually fix the 400+
formatting errors. I will drop this patch and use the use these files
to verify the .clang-format .

Tom

>>
>>> static unsigned short int max_sectors_per_io = MAX_SECTORS_PER_IO;
>>> module_param(max_sectors_per_io, ushort, 0);
>>> -MODULE_PARM_DESC(max_sectors_per_io, "Maximum number of sectors per I/O request (default=MAX_SECTORS_PER_IO=128)");
>>> -
>>> +MODULE_PARM_DESC(
>>> + max_sectors_per_io,
>>> + "Maximum number of sectors per I/O request (default=MAX_SECTORS_PER_IO=128)");
>>>
>>> static unsigned short int max_mbox_busy_wait = MBOX_BUSY_WAIT;
>>> module_param(max_mbox_busy_wait, ushort, 0);
>>> -MODULE_PARM_DESC(max_mbox_busy_wait, "Maximum wait for mailbox in microseconds if busy (default=MBOX_BUSY_WAIT=10)");
>>> +MODULE_PARM_DESC(
>>> + max_mbox_busy_wait,
>>> + "Maximum wait for mailbox in microseconds if busy (default=MBOX_BUSY_WAIT=10)");
>>>
>> This code is longer for no real improvement.
>>
>>> /*
>>> * The File Operations structure for the serial/ioctl interface of the driver
>>> */
>>> static const struct file_operations megadev_fops = {
>>> - .owner = THIS_MODULE,
>>> - .unlocked_ioctl = megadev_unlocked_ioctl,
>>> - .open = megadev_open,
>>> - .llseek = noop_llseek,
>>> + .owner = THIS_MODULE,
>>> + .unlocked_ioctl = megadev_unlocked_ioctl,
>>> + .open = megadev_open,
>>> + .llseek = noop_llseek,
>>> };
>>>
>>> /*
>> Readability loss.
>>
>>> - prod_info_dma_handle = dma_map_single(&adapter->dev->dev,
>>> - (void *)&adapter->product_info,
>>> - sizeof(mega_product_info),
>>> - DMA_FROM_DEVICE);
>>> + prod_info_dma_handle = dma_map_single(
>>> + &adapter->dev->dev, (void *)&adapter->product_info,
>>> + sizeof(mega_product_info), DMA_FROM_DEVICE);
>>>
>> Note the orphaned first parameter and odd indentation.
>>
>>> static DEF_SCSI_QCMD(megaraid_queue)
>>>
>>> -/**
>>> + /**
>>> * mega_allocate_scb()
>>> * @adapter: pointer to our soft state
>>> * @cmd: scsi command from the mid-layer
>> Indentation error.
>>
>>> @@ -418,15 +409,14 @@ static DEF_SCSI_QCMD(megaraid_queue)
>>> * Allocate a SCB structure. This is the central structure for controller
>>> * commands.
>>> */
>>> -static inline scb_t *
>>> -mega_allocate_scb(adapter_t *adapter, struct scsi_cmnd *cmd)
>>> + static inline scb_t *mega_allocate_scb(adapter_t *adapter,
>>> + struct scsi_cmnd *cmd)
>>> {
>>> struct list_head *head = &adapter->free_list;
>> Same.
>>
>>> @@ -586,26 +568,25 @@ mega_build_cmd(adapter_t *adapter, struct scsi_cmnd *cmd, int *busy)
>>>
>>> ldrv_num = mega_get_ldrv_num(adapter, cmd, channel);
>>>
>>> -
>>> max_ldrv_num = (adapter->flag & BOARD_40LD) ?
>>> - MAX_LOGICAL_DRIVES_40LD : MAX_LOGICAL_DRIVES_8LD;
>>> + MAX_LOGICAL_DRIVES_40LD :
>>> + MAX_LOGICAL_DRIVES_8LD;
>>>
>> Churn, if not readability loss. Note the indentation change here is
>> inconsistent with the indentation change noted above.
>>
>>> * 6-byte READ(0x08) or WRITE(0x0A) cdb
>>> */
>>> - if( cmd->cmd_len == 6 ) {
>>> - mbox->m_out.numsectors = (u32) cmd->cmnd[4];
>>> - mbox->m_out.lba =
>>> - ((u32)cmd->cmnd[1] << 16) |
>>> - ((u32)cmd->cmnd[2] << 8) |
>>> - (u32)cmd->cmnd[3];
>>> + if (cmd->cmd_len == 6) {
>>> + mbox->m_out.numsectors = (u32)cmd->cmnd[4];
>>> + mbox->m_out.lba = ((u32)cmd->cmnd[1] << 16) |
>>> + ((u32)cmd->cmnd[2] << 8) |
>>> + (u32)cmd->cmnd[3];
>>>
>>> mbox->m_out.lba &= 0x1FFFFF;
>>>
>> Here, the orphaned term is moved up, next to the =. And yet,
>>
>>> /* Calculate Scatter-Gather info */
>>> - mbox->m_out.numsgelements = mega_build_sglist(adapter, scb,
>>> - (u32 *)&mbox->m_out.xferaddr, &seg);
>>> + mbox->m_out.numsgelements =
>>> + mega_build_sglist(adapter, scb,
>>> + (u32 *)&mbox->m_out.xferaddr,
>>> + &seg);
>>>
>>> return scb;
>>>
>> ... here the first term is moved down and orphaned, which is another
>> inconsistency.
>
>

2022-01-29 11:02:31

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Thu, 2022-01-27 at 16:32 -0800, Tom Rix wrote:
> On 1/27/22 2:47 PM, Nick Desaulniers wrote:
> > + Miguel (the clang-format maintainer), Joe (checkpatch maintainer)
> > These criticisms are worth reviewing.
> >
> > On Thu, Jan 27, 2022 at 2:38 PM Finn Thain <[email protected]> wrote:
> > >
> > > On Thu, 27 Jan 2022, [email protected] wrote:
> > >
> > > > From: Tom Rix <[email protected]>
> > > >
> > > > checkpatch reports several hundred formatting errors. Run these files
> > > > through clang-format and knock off some of them.
> > > >
> > > That method seems like a good recipe for endless churn unless checkpatch
> > > and clang-format really agree about these style rules.
> > >
> > > Why use checkpatch to assess code style, if we could simply diff the
> > > existing source with the output from clang-format... but it seems that
> > > clang-format harms readability, makes indentation errors and uses
> > > inconsistent style rules. Some examples:
>
> Problems with clang-format should be fixed, I'll take a look.
>
> I was reviewing this file for another issue and could not get past how
> horredously bad it was and really did not want to manually fix the 400+
> formatting errors.? I will drop this patch and use the use these files
> to verify the .clang-format .

I think this is more an issue with clang-format than with checkpatch.

If you have specific issues with what checkpatch reports for this
file (or any other file), let me know.

2022-01-29 16:36:34

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On 1/27/22 6:43 PM, Joe Perches wrote:
> On Thu, 2022-01-27 at 16:32 -0800, Tom Rix wrote:
>> On 1/27/22 2:47 PM, Nick Desaulniers wrote:
>>> + Miguel (the clang-format maintainer), Joe (checkpatch maintainer)
>>> These criticisms are worth reviewing.
>>>
>>> On Thu, Jan 27, 2022 at 2:38 PM Finn Thain <[email protected]> wrote:
>>>> On Thu, 27 Jan 2022, [email protected] wrote:
>>>>
>>>>> From: Tom Rix <[email protected]>
>>>>>
>>>>> checkpatch reports several hundred formatting errors. Run these files
>>>>> through clang-format and knock off some of them.
>>>>>
>>>> That method seems like a good recipe for endless churn unless checkpatch
>>>> and clang-format really agree about these style rules.
>>>>
>>>> Why use checkpatch to assess code style, if we could simply diff the
>>>> existing source with the output from clang-format... but it seems that
>>>> clang-format harms readability, makes indentation errors and uses
>>>> inconsistent style rules. Some examples:
>> Problems with clang-format should be fixed, I'll take a look.
>>
>> I was reviewing this file for another issue and could not get past how
>> horredously bad it was and really did not want to manually fix the 400+
>> formatting errors. I will drop this patch and use the use these files
>> to verify the .clang-format .
> I think this is more an issue with clang-format than with checkpatch.
>
> If you have specific issues with what checkpatch reports for this
> file (or any other file), let me know.

Yes, I agree. Its a clang-format problem.

I will be looking to minimize the .clang-format settings to only those
that agree with checkpatch.

Then add settings back in later if their problems can be worked out.

Tom

2022-01-30 16:14:32

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Thu, 2022-01-27 at 21:31 -0800, Tom Rix wrote:
> On 1/27/22 6:43 PM, Joe Perches wrote:
> > On Thu, 2022-01-27 at 16:32 -0800, Tom Rix wrote:
> > > On 1/27/22 2:47 PM, Nick Desaulniers wrote:
> > > > + Miguel (the clang-format maintainer), Joe (checkpatch maintainer)
> > > > These criticisms are worth reviewing.
> > > >
> > > > On Thu, Jan 27, 2022 at 2:38 PM Finn Thain <[email protected]> wrote:
> > > > > On Thu, 27 Jan 2022, [email protected] wrote:
> > > > >
> > > > > > From: Tom Rix <[email protected]>
> > > > > >
> > > > > > checkpatch reports several hundred formatting errors. Run these files
> > > > > > through clang-format and knock off some of them.
> > > > > >
> > > > > That method seems like a good recipe for endless churn unless checkpatch
> > > > > and clang-format really agree about these style rules.
> > > > >
> > > > > Why use checkpatch to assess code style, if we could simply diff the
> > > > > existing source with the output from clang-format... but it seems that
> > > > > clang-format harms readability, makes indentation errors and uses
> > > > > inconsistent style rules. Some examples:
> > > Problems with clang-format should be fixed, I'll take a look.
> > >
> > > I was reviewing this file for another isseue and could not get past how
> > > horredously bad it was and really did not want to manually fix the 400+
> > > formatting errors.? I will drop this patch and use the use these files
> > > to verify the .clang-format .
> > I think this is more an issue with clang-format than with checkpatch.
> >
> > If you have specific issues with what checkpatch reports for this
> > file (or any other file), let me know.
>
> Yes, I agree. Its a clang-format problem.
>
> I will be looking to minimize the .clang-format settings to only those
> that agree with checkpatch.
>
> Then add settings back in later if their problems can be worked out.

Another option would be to use:

./scripts/checkpatch.pl -f --fix[-inplace] [--types=<list>] <files>

where types is an optional list of specific things to change

see:
./scripts/checkpatch.pl --list-types --verbose

to show the possible types.

Only some of these types can be changed with --fix or --fix-inplace

If using checkpatch to change formatting, it sometimes can be useful
to run checkpatch --fix multiple times on the same file as a
checkpatch --fix can create a change than checkpatch will suggest
should itself be fixed.

Of course another option is to do nothing as many will complain,
sometimes senselessly, about 'churn'.

2022-01-30 20:50:03

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Thu, Jan 27, 2022 at 11:47 PM Nick Desaulniers
<[email protected]> wrote:
>
> + Miguel (the clang-format maintainer), Joe (checkpatch maintainer)
> These criticisms are worth reviewing.

Indeed, thanks Nick. In my upgrade to clang-format 11 (from the
current clang-format 4), I will check whether some of these improve.

Cheers,
Miguel

2022-01-31 11:12:10

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On 1/28/22 12:41 AM, Joe Perches wrote:
> On Thu, 2022-01-27 at 21:31 -0800, Tom Rix wrote:
>> On 1/27/22 6:43 PM, Joe Perches wrote:
>>> On Thu, 2022-01-27 at 16:32 -0800, Tom Rix wrote:
>>>> On 1/27/22 2:47 PM, Nick Desaulniers wrote:
>>>>> + Miguel (the clang-format maintainer), Joe (checkpatch maintainer)
>>>>> These criticisms are worth reviewing.
>>>>>
>>>>> On Thu, Jan 27, 2022 at 2:38 PM Finn Thain <[email protected]> wrote:
>>>>>> On Thu, 27 Jan 2022, [email protected] wrote:
>>>>>>
>>>>>>> From: Tom Rix <[email protected]>
>>>>>>>
>>>>>>> checkpatch reports several hundred formatting errors. Run these files
>>>>>>> through clang-format and knock off some of them.
>>>>>>>
>>>>>> That method seems like a good recipe for endless churn unless checkpatch
>>>>>> and clang-format really agree about these style rules.
>>>>>>
>>>>>> Why use checkpatch to assess code style, if we could simply diff the
>>>>>> existing source with the output from clang-format... but it seems that
>>>>>> clang-format harms readability, makes indentation errors and uses
>>>>>> inconsistent style rules. Some examples:
>>>> Problems with clang-format should be fixed, I'll take a look.
>>>>
>>>> I was reviewing this file for another isseue and could not get past how
>>>> horredously bad it was and really did not want to manually fix the 400+
>>>> formatting errors. I will drop this patch and use the use these files
>>>> to verify the .clang-format .
>>> I think this is more an issue with clang-format than with checkpatch.
>>>
>>> If you have specific issues with what checkpatch reports for this
>>> file (or any other file), let me know.
>> Yes, I agree. Its a clang-format problem.
>>
>> I will be looking to minimize the .clang-format settings to only those
>> that agree with checkpatch.
>>
>> Then add settings back in later if their problems can be worked out.
> Another option would be to use:
>
> ./scripts/checkpatch.pl -f --fix[-inplace] [--types=<list>] <files>
>
> where types is an optional list of specific things to change
>
> see:
> ./scripts/checkpatch.pl --list-types --verbose
>
> to show the possible types.
>
> Only some of these types can be changed with --fix or --fix-inplace
>
> If using checkpatch to change formatting, it sometimes can be useful
> to run checkpatch --fix multiple times on the same file as a
> checkpatch --fix can create a change than checkpatch will suggest
> should itself be fixed.
>
> Of course another option is to do nothing as many will complain,
> sometimes senselessly, about 'churn'.

A testsuite for the fixers may help with churn

Any interest or thought on organization ?

I can take stab at one.

Tom

>
>
>

2022-01-31 11:17:07

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

(adding checkpatch reviewers Dwaipayan and Lukas)

Original thread: https://lore.kernel.org/lkml/[email protected]/

On Fri, 2022-01-28 at 07:30 -0800, Tom Rix wrote:
> On 1/28/22 12:41 AM, Joe Perches wrote:
> > Another option would be to use:
> >
> > ./scripts/checkpatch.pl -f --fix[-inplace] [--types=<list>] <files>
> >
> > where types is an optional list of specific things to change
> >
> > see:
> > ./scripts/checkpatch.pl --list-types --verbose
> >
> > to show the possible types.
> >
> > Only some of these types can be changed with --fix or --fix-inplace
> >
> > If using checkpatch to change formatting, it sometimes can be useful
> > to run checkpatch --fix multiple times on the same file as a
> > checkpatch --fix can create a change than checkpatch will suggest
> > should itself be fixed.
> >
> > Of course another option is to do nothing as many will complain,
> > sometimes senselessly, about 'churn'.
>
> A testsuite for the fixers may help with churn

Likely not, as IMO churn is an 'eye of beholder' issue.

> Any interest or thought on organization ?

Not really.

The checkpatch --fix code isn't perfect as it's just regexes.

When writing the --fix code, I've always just used the kernel
sources and git history to inspect/verify suggested changes.

> I can take stab at one.

Enjoy.

2022-01-31 11:26:58

by Bart Van Assche

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On 1/27/22 07:19, [email protected] wrote:
> From: Tom Rix <[email protected]>
>
> checkpatch reports several hundred formatting errors.
> Run these files through clang-format and knock off
> some of them.

Isn't this the kind of patches that carries more risk than value?
Additionally, this patch conflicts with a patch series that I plan to
post soon.

Thanks,

Bart.

2022-01-31 11:31:03

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On 1/28/22 9:42 AM, Bart Van Assche wrote:
> On 1/27/22 07:19, [email protected] wrote:
>> From: Tom Rix <[email protected]>
>>
>> checkpatch reports several hundred formatting errors.
>> Run these files through clang-format and knock off
>> some of them.
>
> Isn't this the kind of patches that carries more risk than value?
> Additionally, this patch conflicts with a patch series that I plan to
> post soon.
>
> Thanks,

I have dropped this patch.

Long term, it would be good have a reliable way to automatically fix
either new files or really broken old files.

So you could do two patches, first would be fixing formatting problems
and second would new stuff.

As it is your new patch will have to both for now.

Tom

>
>
> Bart.
>

2022-01-31 11:31:37

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Fri, 2022-01-28 at 09:59 -0800, Tom Rix wrote:
> On 1/28/22 9:42 AM, Bart Van Assche wrote:
> > On 1/27/22 07:19, [email protected] wrote:
> > > From: Tom Rix <[email protected]>
> > >
> > > checkpatch reports several hundred formatting errors.
> > > Run these files through clang-format and knock off
> > > some of them.
> >
> > Isn't this the kind of patches that carries more risk than value?

Risk for whitespace style reformatting patches is quite low.

Nominally, clang-format changes should not produce a different
compiled object file unless __LINE__/__DATE__/__TIME__ style
changes occur.

If it does, the clang-format tool is broken.

> > Additionally, this patch conflicts with a patch series that I plan to
> > post soon.
[]
> Long term, it would be good have a reliable way to automatically fix
> either new files or really broken old files.

That's really a maintainer preference no?

Especially so for any automation.

2022-01-31 11:32:06

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On 1/28/22 11:11 AM, Joe Perches wrote:
> On Fri, 2022-01-28 at 09:59 -0800, Tom Rix wrote:
>> On 1/28/22 9:42 AM, Bart Van Assche wrote:
>>> On 1/27/22 07:19, [email protected] wrote:
>>>> From: Tom Rix <[email protected]>
>>>>
>>>> checkpatch reports several hundred formatting errors.
>>>> Run these files through clang-format and knock off
>>>> some of them.
>>> Isn't this the kind of patches that carries more risk than value?
> Risk for whitespace style reformatting patches is quite low.
>
> Nominally, clang-format changes should not produce a different
> compiled object file unless __LINE__/__DATE__/__TIME__ style
> changes occur.
>
> If it does, the clang-format tool is broken.
>
>>> Additionally, this patch conflicts with a patch series that I plan to
>>> post soon.
> []
>> Long term, it would be good have a reliable way to automatically fix
>> either new files or really broken old files.
> That's really a maintainer preference no?
>
> Especially so for any automation.

In practice everything is up to the maintainer.

If some maintainer wants fix their formatting then clang-format should
just work

It isn't likely they will have time to hand fix every file.

Tom

>
>

2022-01-31 11:36:01

by Bart Van Assche

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On 1/28/22 11:11, Joe Perches wrote:
> On Fri, 2022-01-28 at 09:59 -0800, Tom Rix wrote:
>> On 1/28/22 9:42 AM, Bart Van Assche wrote:
>>> Isn't this the kind of patches that carries more risk than value?
>
> Risk for whitespace style reformatting patches is quite low.

But the annoyance factor for patches that reformat an entire source file
is very high. It forces everyone who has conflicting patches pending to
wait until the reformatting patch went in, rebase their tree and also to
resolve the complicated merge conflicts that are the result of the
reformatting patch. I have already had to do this several times in the past.

Thanks,

Bart.

2022-01-31 11:49:47

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Fri, 28 Jan 2022, Tom Rix wrote:

> A testsuite for the fixers may help with churn
>
> Any interest or thought on organization ?
>

Here's one way:

1. Identify some source files that are exemplary (i.e. good style).
2. Run a new script to introduce style mistakes into those files.
3. Run the Reformatter Under Test on those files.
4. Compare the output with the original files.

But that's not sufficient to prevent more style patches (churn). We still
need a way to avoid wasting everyone's time writing, submitting, reviewing
and merging the inevitably incomplete style patches that may contain
regressions and cause conflicts.

The tooling I would like to see is not a script to introduce style
mistakes but an editor for binaries, such that C is not the "source" at
all but only an ephemeral representation to make it easier for humans to
effect changes to binaries (and debug info, commentary etc.)

Nonetheless, perfecting the clang-format tool is a good step towards a
better workflow, which I described once before.
https://lore.kernel.org/r/[email protected]/

2022-03-02 23:44:21

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On 1/28/22 11:37 AM, Tom Rix wrote:
>
> On 1/28/22 11:11 AM, Joe Perches wrote:
>> On Fri, 2022-01-28 at 09:59 -0800, Tom Rix wrote:
>>> On 1/28/22 9:42 AM, Bart Van Assche wrote:
>>>> On 1/27/22 07:19, [email protected] wrote:
>>>>> From: Tom Rix <[email protected]>
>>>>>
>>>>> checkpatch reports several hundred formatting errors.
>>>>> Run these files through clang-format and knock off
>>>>> some of them.
>>>> Isn't this the kind of patches that carries more risk than value?
>> Risk for whitespace style reformatting patches is quite low.
>>
>> Nominally, clang-format changes should not produce a different
>> compiled object file unless __LINE__/__DATE__/__TIME__ style
>> changes occur.
>>
>> If it does, the clang-format tool is broken.
>>
>>>> Additionally, this patch conflicts with a patch series that I plan to
>>>> post soon.
>> []
>>> Long term, it would be good have a reliable way to automatically fix
>>> either new files or really broken old files.
>> That's really a maintainer preference no?
>>
>> Especially so for any automation.
>
> In practice everything is up to the maintainer.
>
> If some maintainer wants fix their formatting then clang-format should
> just work
>
> It isn't likely they will have time to hand fix every file.

A follow up issue in the clang project has been raised by Konrad, here

https://github.com/llvm/llvm-project/issues/54137

Tom

>
> Tom
>
>>
>>

2022-03-03 08:39:15

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Wed, 2 Mar 2022, Tom Rix wrote:

> >>> Long term, it would be good have a reliable way to automatically fix
> >>> either new files or really broken old files.
> >> That's really a maintainer preference no?
> >>
> >> Especially so for any automation.
> >
> > In practice everything is up to the maintainer.
> >
> > If some maintainer wants fix their formatting then clang-format should
> > just work
> >
> > It isn't likely they will have time to hand fix every file.
>
> A follow up issue in the clang project has been raised by Konrad, here
>
> https://github.com/llvm/llvm-project/issues/54137
>

Why request a "leave" option for every style rule? Why not just a "leave"
option for the most contentious rules?

The response from the developers that anyone who wants to leave existing
code unmolested by certain rules should "wake up and smell the coffee" is
obnoxious, IMO.

Presumably clang-format must grow until it has sufficient program logic
and config options to cater to every exception to every rule. How long
will that take? Some carefully chosen "leave" options might make the
program much more useful in the near term.

> Tom
>
>
> >
> > Tom
> >
> >>
> >>
>

2022-03-03 18:35:00

by Konrad Wilhelm Kleine

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

(Resending in plain text format to make mailing lists happy.)

Hi Finn,

On Thu, 3 Mar 2022 at 09:40, Finn Thain <[email protected]> wrote:
>
>
> On Wed, 2 Mar 2022, Tom Rix wrote:
>
> > >>> Long term, it would be good have a reliable way to automatically fix
> > >>> either new files or really broken old files.
> > >> That's really a maintainer preference no?
> > >>
> > >> Especially so for any automation.
> > >
> > > In practice everything is up to the maintainer.
> > >
> > > If some maintainer wants fix their formatting then clang-format should
> > > just work
> > >
> > > It isn't likely they will have time to hand fix every file.
> >
> > A follow up issue in the clang project has been raised by Konrad, here
> >
> > https://github.com/llvm/llvm-project/issues/54137
> >
>
> Why request a "leave" option for every style rule? Why not just a "leave"
> option for the most contentious rules?

Getting to the point that every style option can be disabled
individually is not an operation done in one go. I plan on presenting
the changes required to exactly one style option and from there I'm
all ears if you have style options that you consider "contentious". We
could certainly tackle them next. But for starters I think it's fine
to just show the impact of changing one style option only.

>> The response from the developers that anyone who wants to leave existing
>> code unmolested by certain rules should "wake up and smell the coffee" is
>> obnoxious, IMO.

I hear you, I like tea better than coffee ;). In all seriousness, I'm
here to help and I don't judge the developers for protecting their
code and having an opinion about other projects. We're here to talk
and find a solution, nothing more. Let's try to ignore the sarcastic
undertone in the dialogue.

>> Presumably clang-format must grow until it has sufficient program logic
>> and config options to cater to every exception to every rule. How long
>> will that take? Some carefully chosen "leave" options might make the
>> program much more useful in the near term.

That's what I aim at. I hope I've outlined this in the first paragraph
of this email. One baby step at a time...
- Konrad

>
>
>
> > Tom
> >
> >
> > >
> > > Tom
> > >
> > >>
> > >>
> >
>

2022-03-03 23:32:05

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Thu, Mar 3, 2022 at 11:44 PM Finn Thain <[email protected]> wrote:
>
> Others might argue that they should always be changed from,
>
> /*
> * this style
> * of multiline comment
> */
>
> to
>
> /* this style
> * of multiline comment
> */

In general, for things that the coding style guide talks about, we
should follow them, even if some subsystems do not (they can always
override in their folder if they really, really want it). So, here for
instance, the first one should be used.

Cheers,
Miguel

2022-03-04 02:42:19

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

Hi Konrad,

On Thu, 3 Mar 2022, Konrad Wilhelm Kleine wrote:

> On Thu, 3 Mar 2022 at 09:40, Finn Thain <[email protected]> wrote:
>
> > On Wed, 2 Mar 2022, Tom Rix wrote:
> >
> > > >>> Long term, it would be good have a reliable way to automatically
> > > >>> fix either new files or really broken old files.
> > > >> That's really a maintainer preference no?
> > > >>
> > > >> Especially so for any automation.
> > > >
> > > > In practice everything is up to the maintainer.
> > > >
> > > > If some maintainer wants fix their formatting then clang-format
> > > > should just work
> > > >
> > > > It isn't likely they will have time to hand fix every file.
> > >
> > > A follow up issue in the clang project has been raised by Konrad,
> > > here
> > >
> > > https://github.com/llvm/llvm-project/issues/54137
> > >
> >
> > Why request a "leave" option for every style rule? Why not just a
> > "leave" option for the most contentious rules?
> >
>
> Getting to the point that every style option can be disabled
> individually is not an operation done in one go. I plan on presenting
> the changes required to exactly one style option and from there I'm all
> ears if you have style options that you consider "contentious".

Sure, I'll provide an example of that below in case you're interested in
what I happen to find contentious. But this isn't about my taste.

The issue is really about removing an obstacle to wider adoption of
clang-formt. Therefore, shouldn't you be asking, what style rules have
already proven to be contentious within the project?

You can look to kernel subsystem style rules for examples of that. E.g.
some might argue that comments should always be left unmolested (where
they exist for the benefit of human readers and not tooling).

Others might argue that they should always be changed from,

/*
* this style
* of multiline comment
*/

to

/* this style
* of multiline comment
*/

For another example, the mailing lists recently saw another style rule
difference between subsystems:
https://lore.kernel.org/all/20220301055231.GI2812@kadam/

Joe said, in effect, "leave" whereas others seem to have different views.

Finally, here's an example that I personally found contentious.
drivers/scsi/NCR5380.c line 2306:
dsprintk(NDEBUG_ABORT, instance,
"abort: removed %p from sense queue\n", cmd);

Note the spaces (for alignment) following the tabs (for scope). I mention
this example not because I expect the world to "wake up and smell the
coffee" one day and embrace spaces-after-tabs.

I mention it because I expect the tabs/spaces/both issue to remain
contentious indefinitely (within any sufficiently large project).

I can think of another good example (line wrap) but I'll stop here.

2022-03-04 08:49:04

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Thu, 2022-03-03 at 21:22 -0800, Bart Van Assche wrote:
> On 3/3/22 15:38, Joe Perches wrote:
> > One argument is that churn leads to difficulty in backporting
> > fixes to older 'stable' versions.
> >
> > I think the churn argument is overstated.
>
> I'm often backporting patches to older kernels and I think the churn argument
> has not been emphasized enough. Backporting patches is a normal aspect of a
> product lifecycle since a kernel version is chosen when development of a
> product starts and bugfixes are cherry-picked from upstream selectively.

Stable backporting is almost certainly what people get _paid_ to do.

IMO the most important tree is the current one not backports to stable.

2022-03-04 09:36:34

by Bart Van Assche

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On 3/3/22 15:38, Joe Perches wrote:
> One argument is that churn leads to difficulty in backporting
> fixes to older 'stable' versions.
>
> I think the churn argument is overstated.

I'm often backporting patches to older kernels and I think the churn argument
has not been emphasized enough. Backporting patches is a normal aspect of a
product lifecycle since a kernel version is chosen when development of a
product starts and bugfixes are cherry-picked from upstream selectively.

Thanks,

Bart.

2022-03-04 10:10:28

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Fri, 2022-03-04 at 00:17 +0100, Miguel Ojeda wrote:
> On Thu, Mar 3, 2022 at 11:44 PM Finn Thain <[email protected]> wrote:
> >
> > Others might argue that they should always be changed from,
> >
> > /*
> > * this style
> > * of multiline comment
> > */
> >
> > to
> >
> > /* this style
> > * of multiline comment
> > */
>
> In general, for things that the coding style guide talks about, we
> should follow them, even if some subsystems do not (they can always
> override in their folder if they really, really want it). So, here for
> instance, the first one should be used.

It's up to individual maintainers to each decide on what might be
considered unnecessary churn for the subsystems they control.

One argument is that churn leads to difficulty in backporting
fixes to older 'stable' versions.

I think the churn argument is overstated.

2022-03-04 15:23:02

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Fri, Mar 4, 2022 at 2:46 PM Tom Rix <[email protected]> wrote:
>
> The churn level will be very high.

Nobody is planning to reformat the entire kernel, at least not until
the tool is close enough to the kernel style, which will take a while.

> Until clang-format has an opt-in mechanism, I do not think clang-format
> should be used.
>
> .clang-format should be moved to staging/ to reflect its not being ready
> status.

What is not ready about it?

It is a fairly useful tool that some people is already using, either
for full file formatting or for sections within their editor.

Cheers,
Miguel

2022-03-04 16:03:09

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Thu, 3 Mar 2022, Joe Perches wrote:

> On Fri, 2022-03-04 at 00:17 +0100, Miguel Ojeda wrote:
> > On Thu, Mar 3, 2022 at 11:44 PM Finn Thain wrote:
> > >
> > > Others might argue that they should always be changed from,
> > >
> > > /*
> > > * this style
> > > * of multiline comment
> > > */
> > >
> > > to
> > >
> > > /* this style
> > > * of multiline comment
> > > */
> >
> > In general, for things that the coding style guide talks about, we
> > should follow them, even if some subsystems do not (they can always
> > override in their folder if they really, really want it). So, here for
> > instance, the first one should be used.
>
> It's up to individual maintainers to each decide on what might be
> considered unnecessary churn for the subsystems they control.
>
> One argument is that churn leads to difficulty in backporting fixes to
> older 'stable' versions.
>
> I think the churn argument is overstated.
>

If you would have clang-format override the committer and retrospectively
apply subsystem style rules (rather than the rules used by a majority of
subsystems or those preferred by Linus for example) it would add friction
to code re-use, movement, comparison, any tree-wide program
transformation, and also subsystem boundary changes.

Per-subsystem style rules are inherently contentious and therefore good
candidates for the "leave alone" functionality discussed in the issue
tracker.

2022-03-04 19:10:52

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Fri, Mar 4, 2022 at 6:37 PM Joe Perches <[email protected]> wrote:
>
> I rather doubt clang-format will ever be 'close enough'.
>
> A human's sense of 'taste' for reading code is very different than
> what an automated tool produces.

Maybe, but it is a trade-off. If it is close enough, the benefits of
automatic formatting may overcome the downsides.

> Also, try looking at the changes clang-format does on a file chosen
> at random:
>
> o columnarized definitions -> not columnarized
> o odd line continuation placement using spaces and not tabs before \
> o odd array definition layouts
> o per line definitions with comments poorly laid out
> o individual line definitions rewrapped
> o enum definitions on multiple lines compressed to single lines
> o u8 array definition layouts where the first element has a separate
> meaning than the subsequent elements are compressed and made
> difficult to understand

I am not sure what you are trying to show here -- some of these are
precisely the things that the tool could improve or have already
improved, and we may just need to use the new option.

For instance, for the columnarized macros case, it is possible to
align them since clang-format 9. For array of structures, there is
also a new alignment option since clang-format 13. Etc.

For the wrapping and related bits, now that the limit on 80 is a bit
more fuzzy, we could perhaps tweak the penalties to improve the
decision making.

In summary, what we should be trying to do is improve the tool
configuration and tool itself to see if we can get it to be close
enough to the kernel style to make it more widely used.

> I think _some_ clang-format output is ok, but the concept of
> enabling/disabling specific reformatting bits would be quite useful.
>
> And sprinkling "clang-format on/off" lines in the code is not good.

Definitely, but it is fine in some exceptional cases.

> Any control codes that determine when source code layout might be
> immutable or allowed to be modified could be should be tool name
> agnostic.

I don't see why would that be a problem, and I don't understand why we
would use several different formatting tools (the point is to be
consistent, after all); but sure, we could propose an alternative
spelling.

Cheers,
Miguel

2022-03-04 19:12:06

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Fri, Mar 4, 2022 at 5:03 AM Joe Perches <[email protected]> wrote:
>
> It's up to individual maintainers to each decide on what might be
> considered unnecessary churn for the subsystems they control.
>
> One argument is that churn leads to difficulty in backporting
> fixes to older 'stable' versions.
>
> I think the churn argument is overstated.

I was only pointing out that, for the global `.clang-format` file, the
style in `coding-style.rst` should be followed. Then subsystem
maintainers may override it if really needed.

I definitely agree that we should aim to have a consistent style.
However, by allowing a few exceptions, some major subsystems may start
using `clang-format` sooner.

Cheers,
Miguel

2022-03-04 20:01:03

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On 3/3/22 3:38 PM, Joe Perches wrote:
> On Fri, 2022-03-04 at 00:17 +0100, Miguel Ojeda wrote:
>> On Thu, Mar 3, 2022 at 11:44 PM Finn Thain <[email protected]> wrote:
>>> Others might argue that they should always be changed from,
>>>
>>> /*
>>> * this style
>>> * of multiline comment
>>> */
>>>
>>> to
>>>
>>> /* this style
>>> * of multiline comment
>>> */
>> In general, for things that the coding style guide talks about, we
>> should follow them, even if some subsystems do not (they can always
>> override in their folder if they really, really want it). So, here for
>> instance, the first one should be used.
> It's up to individual maintainers to each decide on what might be
> considered unnecessary churn for the subsystems they control.

clang-format does not have an opt-in mechanism like indent, it is
all-or-nothing

What is done is all the settings in .clang-format and the default settings.

The churn level will be very high.

Until clang-format has an opt-in mechanism, I do not think clang-format
should be used.

.clang-format should be moved to staging/ to reflect its not being ready
status.

staging/ would be a good long term home. it's content should not need
backporting and is more likely to have style issues.

Tom

>
> One argument is that churn leads to difficulty in backporting
> fixes to older 'stable' versions.
>
> I think the churn argument is overstated.
>
>

2022-03-04 20:26:49

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Fri, 2022-03-04 at 15:02 +0100, Miguel Ojeda wrote:
> On Fri, Mar 4, 2022 at 2:46 PM Tom Rix <[email protected]> wrote:
> >
> > The churn level will be very high.
>
> Nobody is planning to reformat the entire kernel, at least not until
> the tool is close enough to the kernel style, which will take a while.

I rather doubt clang-format will ever be 'close enough'.

A human's sense of 'taste' for reading code is very different than
what an automated tool produces.

For instance, clang-format does a quite poor job when comments are
intermixed with definitions.

Also, try looking at the changes clang-format does on a file chosen
at random:

$ clang-format -i drivers/hid/hid-sony.c
$ git diff drivers/hid/hid-sony.c
[...]

o columnarized definitions -> not columnarized
o odd line continuation placement using spaces and not tabs before \
o odd array definition layouts
o per line definitions with comments poorly laid out
o individual line definitions rewrapped
o enum definitions on multiple lines compressed to single lines
o u8 array definition layouts where the first element has a separate
meaning than the subsequent elements are compressed and made
difficult to understand

I think _some_ clang-format output is ok, but the concept of
enabling/disabling specific reformatting bits would be quite useful.

And sprinkling "clang-format on/off" lines in the code is not good.

Any control codes that determine when source code layout might be
immutable or allowed to be modified could be should be tool name
agnostic.

2022-03-04 21:05:24

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Fri, 2022-03-04 at 19:48 +0100, Miguel Ojeda wrote:
> On Fri, Mar 4, 2022 at 6:37 PM Joe Perches <[email protected]> wrote:
> >
> > I rather doubt clang-format will ever be 'close enough'.
> >
> > A human's sense of 'taste' for reading code is very different than
> > what an automated tool produces.
>
> Maybe,

Hey again Miguel.

Is that statement really disputable?

> but it is a trade-off. If it is close enough, the benefits of
> automatic formatting may overcome the downsides.

IYO. I think using an SCCS with better language understanding rather
than a line oriented one could be an improvement. Such a tool could
allow arbitrary style reformatting at check-in/check-out.

> > Also, try looking at the changes clang-format does on a file chosen
> > at random:
> >
> > o columnarized definitions -> not columnarized
> > o odd line continuation placement using spaces and not tabs before \
> > o odd array definition layouts
> > o per line definitions with comments poorly laid out
> > o individual line definitions rewrapped
> > o enum definitions on multiple lines compressed to single lines
> > o u8 array definition layouts where the first element has a separate
> > meaning than the subsequent elements are compressed and made
> > difficult to understand
>
> I am not sure what you are trying to show here -- some of these are
> precisely the things that the tool could improve or have already
> improved, and we may just need to use the new option.

All of these existing code are more human readable than the code
reformatted using clang-format.

I used whatever is the latest clang-format here with today's -next.
https://releases.llvm.org/download.html

> For instance, for the columnarized macros case, it is possible to
> align them since clang-format 9. For array of structures, there is
> also a new alignment option since clang-format 13. Etc.

Then perhaps you as the maintainer of the kernel's .clang-format file
could update the entries for those new options.

I believe the minimum clang version is already 11. Maybe higher.
I don't track clang or use it very much. The clang version I use
though is 13.

> For the wrapping and related bits, now that the limit on 80 is a bit
> more fuzzy, we could perhaps tweak the penalties to improve the
> decision making.

Please have at it.
But perhaps tweaking will just improve some cases and worsen others.

> In summary, what we should be trying to do is improve the tool
> configuration and tool itself to see if we can get it to be close
> enough to the kernel style to make it more widely used.
>
> > I think _some_ clang-format output is ok, but the concept of
> > enabling/disabling specific reformatting bits would be quite useful.
> >
> > And sprinkling "clang-format on/off" lines in the code is not good.
>
> Definitely, but it is fine in some exceptional cases.

I don't think so.

> > Any control codes that determine when source code layout might be
> > immutable or allowed to be modified could be should be tool name
> > agnostic.
>
> I don't see why would that be a problem, and I don't understand why we
> would use several different formatting tools (the point is to be
> consistent, after all); but sure, we could propose an alternative
> spelling.

Thanks.

There is no "one true editor".
There will not be "one true source code formatter" either.

cheers not jeers, just keep at it. Joe

2022-03-04 21:06:12

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Fri, Mar 4, 2022 at 8:28 PM Joe Perches <[email protected]> wrote:
>
> Is that statement really disputable?

By "Maybe", I only mean that you may be right (or not), i.e. we cannot
really show one way or the other with data.

But note that many people (including projects, companies and even
programming languages) prefer consistency and automated formatting vs.
the alternative.

> IYO. I think using an SCCS with better language understanding rather
> than a line oriented one could be an improvement. Such a tool could
> allow arbitrary style reformatting at check-in/check-out.

clang-format is not line oriented. In fact, you may be able to get
quite close to what you suggest with clang-format if you have a
project that is automatically formatted.

> All of these existing code are more human readable than the code
> reformatted using clang-format.

That is subjective (for some it may be good enough), and anyway you
compared it to the current config file (clang-format 4). The point I
raised is that clang-format 13 and future versions should be closer to
what you expect.

And by saying "I rather doubt clang-format will ever be 'close
enough'.", then there is little incentive for the clang-format team to
actually add support the things we need...

> I used whatever is the latest clang-format here with today's -next.
> https://releases.llvm.org/download.html

I think this may be the source of confusion -- even if you use
clang-format 13, you need to edit the config file to use the new
options.

In other words, when I said something is supported since e.g.
clang-format 9, one still needs to enable it in the config file.

> Then perhaps you as the maintainer of the kernel's .clang-format file
> could update the entries for those new options.

No, I cannot, because that is an error, thus breaks users of old
clang-format versions.

That is why I suggested to track the minimum LLVM supported version in
the kernel, i.e. go at least to LLVM 11. And then over time keep
upgrading it and getting better output.

> I believe the minimum clang version is already 11. Maybe higher.
> I don't track clang or use it very much. The clang version I use
> though is 13.

Yes, I am aware, please see above. The issue and why I did not "just
do it" is that people could have been using clang-format from e.g.
their distro, but compiling with GCC.

> But perhaps tweaking will just improve some cases and worsen others.

Yes, of course.

> I don't think so.

Why?

> There is no "one true editor".
> There will not be "one true source code formatter" either.

Not for every project, but for a single project, there can definitely
be a "single formatter". In fact, that is what many other projects do
in practice in order to reach a point where automatic formatting is
done.

Cheers,
Miguel

2022-03-04 23:06:43

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On 3/4/22 6:02 AM, Miguel Ojeda wrote:
> On Fri, Mar 4, 2022 at 2:46 PM Tom Rix <[email protected]> wrote:
>> The churn level will be very high.
> Nobody is planning to reformat the entire kernel, at least not until
> the tool is close enough to the kernel style, which will take a while.

The churn on a single file like this is high.

There is no way to separate just one type of change, you get all the
changes.

Not having an opt-in/out per type of change is a flaw that makes
clang-format not ready.

Tom

>
>> Until clang-format has an opt-in mechanism, I do not think clang-format
>> should be used.
>>
>> .clang-format should be moved to staging/ to reflect its not being ready
>> status.
> What is not ready about it?
>
> It is a fairly useful tool that some people is already using, either
> for full file formatting or for sections within their editor.
>
> Cheers,
> Miguel
>

2022-03-05 05:10:51

[permalink] [raw]

Subject: Re: [PATCH] scsi: megaraid: cleanup formatting of megaraid

On Fri, 4 Mar 2022, Joe Perches wrote:

>
> Is that statement really disputable?
>

If there was such a thing as an indisputable statement, a set of
universally accepted style rules would soon follow, I suspect.

...

>
> There is no "one true editor".
> There will not be "one true source code formatter" either.
>

It only has to be sufficiently good that it produces a consensus.

2022-03-05 08:57:04