lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  linux-cve-announce  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:	Fri, 19 Feb 2016 09:43:47 +0100
From:	Peter Zijlstra <peterz@...radead.org>
To:	Ingo Molnar <mingo@...nel.org>
Cc:	Tony Luck <tony.luck@...el.com>, linux-kernel@...r.kernel.org,
	Thomas Gleixner <tglx@...utronix.de>,
	"H. Peter Anvin" <hpa@...or.com>, Borislav Petkov <bp@...en8.de>,
	Linus Torvalds <torvalds@...ux-foundation.org>,
	Andrew Morton <akpm@...ux-foundation.org>
Subject: Re: [PATCH v11 3/4] x86, mce: Add __mcsafe_copy()

On Fri, Feb 19, 2016 at 08:58:43AM +0100, Ingo Molnar wrote:
> > I prefer to use my modern console width to display multiple columns of text, 
> > instead of wasting it to display mostly whitespace. Therefore I still very much 
> > prefer ~80 char wide code.
> 
> Btw., the main reason I hate the col80 limit is that I see such patches 
> frequently:
> 
>  void pcibios_add_bus(struct pci_bus *bus)
>  {
> +#ifdef CONFIG_DMI
> +       const struct dmi_device *dmi;
> +       struct dmi_dev_onboard *dslot;
> +       char sname[128];
> +
> +       dmi = NULL;
> +       while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEV_SLOT,
> +                                     NULL, dmi)) != NULL) {
> +               dslot = dmi->device_data;
> +               if (dslot->segment == pci_domain_nr(bus) &&
> +                   dslot->bus == bus->number) {
> +                       dev_info(&bus->dev, "Found SMBIOS Slot %s\n",
> +                                dslot->dev.name);
> +                       snprintf(sname, sizeof(sname), "%s-%d",
> +                                dslot->dev.name,
> +                                dslot->instance);
> +                       pci_create_slot(bus, dslot->devfn,
> +                                       sname, NULL);
> +               }
> +       }
> +#endif
>         acpi_pci_add_bus(bus);
> 
> Which gobbledygook has 6 (!) col80 artifacts - and it's a pretty straightforward 
> piece of code with just 2 levels of indentation.
> 
> It is IMHO much more readable in the following form:
> 
>  void pcibios_add_bus(struct pci_bus *bus)
>  {
>  #ifdef CONFIG_DMI
>         const struct dmi_device *dmi;
>         struct dmi_dev_onboard *dslot;
>         char sname[128];
> 
>         dmi = NULL;
>         while ((dmi = dmi_find_device(DMI_DEV_TYPE_DEV_SLOT, NULL, dmi)) != NULL) {
>                 dslot = dmi->device_data;
>                 if (dslot->segment == pci_domain_nr(bus) && dslot->bus == bus->number) {
>                         dev_info(&bus->dev, "Found SMBIOS Slot %s\n", dslot->dev.name);
>                         snprintf(sname, sizeof(sname), "%s-%d", dslot->dev.name, dslot->instance);
>                         pci_create_slot(bus, dslot->devfn, sname, NULL);
>                 }
>         }
>  #endif
>         acpi_pci_add_bus(bus);
> 
> BYMMV.

So I mostly agree, although your example does wrap on my normal display
width. The thing is though, we have to have a limit, otherwise people
will completely let loose and we'll end up with lines >200 chars wide
(I've worked on code like that in the past, and its a right pain).

And 80 has so far mostly worked just fine. Its just that people seem
unable to take guidelines as just than, and instead produce the most
horrible code just to adhere to checkpatch or whatnot.

And I'd much rather have an extra column of code than waste a lot of
screen-estate to display mostly whitespace.

Also, this 'artificial' limit on indentation level does in general
encourage people to write saner code.

Powered by blists - more mailing lists

Powered by Openwall GNU/*/Linux Powered by OpenVZ