1254918 - clang-format NSS: cmd

Status: RESOLVED FIXED

(NSS :: Libraries, defect)

Description

[Martin Thomson [:mt:]]

These two directories produce new warnings in gcc 6, which detects bad indentation, e.g.,

vfychain.c: In function ‘main’:
vfychain.c:442:59: warning: statement is indented as if it were guarded by... [-Wmisleading-indentation]
             if (secStatus != SECSuccess) Usage(progName); break;
                                                           ^~~~~
vfychain.c:442:13: note: ...this ‘if’ clause, but it is not
             if (secStatus != SECSuccess) Usage(progName); break;
             ^~

Applying clang-format should clean that right up.

Comment 1

[Franziskus Kiefer [:franziskus]]

Attachment: CmdClangFormat.patch

clang-format on cmd.

This is on all of cmd and also fixes the gcc6 build errors.

Note that bltest and pk12util don't produce identical binaries. In the case of bltest it's a macro using __LINE__ again. For pk12util I'm not really sure where the differences are coming from.

Comment 2

[Kai Engert (:KaiE:)]

Comment on attachment 8730311 [details] [diff] [review]
CmdClangFormat.patch

There are several multi-line comments which are ugly.
I wish there was a solution.
(Please don't try to fix them manually, the patch is too large for that, IMHO.)

There are a few lines that are too long.
You can use wc -L to display the longest line of a file, in your patch, it's 776 !
r- for that.

So, given the huge size of this patch, I think it's too painful to go through it manually to fix the issues. Can you think of a more automated way to fix that?

Comment 3

[Kai Engert (:KaiE:)]

Comment on attachment 8730311 [details] [diff] [review]
CmdClangFormat.patch

Hmm, I just found, you aren't introducing that very long line, it's already present in certcgi.c

Nevertheless, there are still some other lone lines.

in blapitest.c, 207 chars:
 ioMode = (bltest.options[opt_B64].activated) ...

in pk11table.c, 173 chars
 { "Unload", F_Unload, "Unload the currrently loaded PKCS #11 library\n" ...


I think we should try to avoid introducing such long lines.
The tables in pk11table.c should probably be protected with wrappers.

Could you please try to go through the patch yourself and minimize these long lines?

Comment 4

[Kai Engert (:KaiE:)]

FYI, code to print longest line in file:
cat CmdClangFormat.patch | awk ' { if ( length > x ) { x = length; y = $0 } }END{ print y }'

found on http://unix.stackexchange.com/questions/24509/how-to-print-the-longest-line-in-a-file

Comment 5

[Franziskus Kiefer [:franziskus]]

Thanks Kai.

I tried a different clang-format version and missed some of the long lines. Will fix them.

> There are several multi-line comments which are ugly.
> I wish there was a solution.
> (Please don't try to fix them manually, the patch is too large for that, IMHO.)

This is the main problem I have with clang-format and I'm not sure if/how we can fix it. I usually fix them manually, that works and most of them should stay the same if applying clang-format again, but not all. clang-format doesn't touch comments (except for adding "\" if in a preprocessor command) and thus doesn't even replace tabs. The only way we can fix this would be to fix clang-format I think.

Comment 6

[Franziskus Kiefer [:franziskus]]

Attachment: CmdClangFormat.patch

comments are still sometimes a bit weird, but I didn't find anything to fix this other than manual. But there shouldn't be any too long lines anymore, everything like in the other patches.

Comment 7

[Martin Thomson [:mt:]]

Comment on attachment 8733453 [details] [diff] [review]
CmdClangFormat.patch

Review of attachment 8733453 [details] [diff] [review]:
-----------------------------------------------------------------

I skimmed through a few of these.  I think that you can easily fix the long lines Kai found.

I wonder if we could do this in stages to avoid reviewer fatigue.

::: cmd/addbuiltin/addbuiltin.c
@@ +268,5 @@
> +           "# This Source Code Form is subject to the terms of the Mozilla Public\n"
> +           "# License, v. 2.0. If a copy of the MPL was not distributed with this\n"
> +           "# file, You can obtain one at http://mozilla.org/MPL/2.0/.\n"
> +           "#\n"
> +           "CVS_ID \"@(#) $RCSfile$ $Revision$ $Date$\"\n"

This doesn't seem to be necessary any more.  Maybe just delete it.

::: cmd/bltest/blapitest.c
@@ +985,5 @@
> +            break;
> +        case bltestHexSpaceDelim:
> +            SECITEM_AllocItem(arena, &input->buf, in->len / 5);
> +            for (i = 0, j = 0; i <
> +                               in->len;

Odd break here.

@@ +3585,5 @@
>  };
>  
>  static secuCommandFlag bltest_commands[] =
> +    {
> +      { /* cmd_Decrypt  */ 'D', PR_FALSE, 0, PR_FALSE },

Can you fix the whitespace in the comments in these two tables?  A single space should suffice.

::: cmd/certcgi/certcgi.c
@@ +2036,2 @@
>  #ifdef OFFLINE
> +    char *form_output = "key=MIIBPTCBpzCBnzANBgkqhkiG9w0BAQEFAAOBjQAwgYkCgYEA7SLqjWBL9Wl11Vlg%0AaMqZCvcQOL%2FnvSqYPPRP0XZy9SoAeyWzQnBOiCm2t8H5mK7r2jnKdAQOmfhjaJil%0A3hNVu3SekHOXF6Ze7bkWa6%2FSGVcY%2FojkydxFSgY43nd1iydzPQDp8WWLL%2BpVpt%2B%2B%0ATRhFtVXbF0fQI03j9h3BoTgP2lkCAwEAARYDZm9vMA0GCSqGSIb3DQEBBAUAA4GB%0AAJ8UfRKJ0GtG%2B%2BufCC6tAfTzKrq3CTBHnom55EyXcsAsv6WbDqI%2F0rLAPkn2Xo1r%0AnNhtMxIuj441blMt%2Fa3AGLOy5zmC7Qawt8IytvQikQ1XTpTBCXevytrmLjCmlURr%0ANJryTM48WaMQHiMiJpbXCqVJC1d%2FpEWBtqvALzZaOOIy&subject=CN%3D%22test%22%26serial-auto%3Dtrue%26serial_value%3D%26ver-1%3Dtrue%26ver-3%3Dfalse%26caChoiceradio-SignWithDefaultkey%3Dtrue%26caChoiceradio-SignWithRandomChain%3Dfalse%26autoCAs%3D%26caChoiceradio-SignWithSpecifiedChain%3Dfalse%26manCAs%3D%26%24";

This needs fixing badly.

Comment 8

[Kai Engert (:KaiE:)]

Comment on attachment 8733453 [details] [diff] [review]
CmdClangFormat.patch

r=kaie

I skimmed through this patch very quickly (keeping page down pressed, trying to catch bad looking things).

I think we should check it in, even if it isn't perfect, before it rots. Luckily, it still applies fine.

The long line is pre-existing, so we shouldn't require fixing as part of the reformat.

Nevertheless, it would be nice to fix the few issues that Martin found, in a follow-up patch. That should be doable quickly.

I saw a few tables with numbers that were reformatted, but it didn't seem worse after the reformat.

There are a few initialized tables, where the first member of the table isn't on a new line, but on the first line, together with the variable name and the opening "= {", which causes the whole table to be indented with e.g. 60 chars.

But the lines I saw aren't very long. So, I'm OK to accept those.

Comment 9

[Kai Engert (:KaiE:)]

(In reply to Franziskus Kiefer [:fkiefer or :franziskus] from comment #6)
> Created attachment 8733453 [details] [diff] [review]
> CmdClangFormat.patch

https://hg.mozilla.org/projects/nss/rev/e812f2193f12

Comment 10

[Kai Engert (:KaiE:)]

Attachment: address Martin's comment 7 - incremental patch

Comment 11

[Martin Thomson [:mt:]]

Comment on attachment 8744026 [details] [diff] [review]
address Martin's comment 7 - incremental patch

Review of attachment 8744026 [details] [diff] [review]:
-----------------------------------------------------------------

Only one nit (125 characters is still a little long for me).

::: cmd/certcgi/certcgi.c
@@ +2039,5 @@
> +        "QI03j9h3BoTgP2lkCAwEAARYDZm9vMA0GCSqGSIb3DQEBBAUAA4GB%0AAJ8UfRKJ0GtG%2B%2BufCC6tAfTzKrq3CTBHnom55EyXcsAsv6WbDqI%2F0rLAPkn2"
> +        "Xo1r%0AnNhtMxIuj441blMt%2Fa3AGLOy5zmC7Qawt8IytvQikQ1XTpTBCXevytrmLjCmlURr%0ANJryTM48WaMQHiMiJpbXCqVJC1d%2FpEWBtqvALzZaOOIy"
> +        "&subject=CN%3D%22test%22%26serial-auto%3Dtrue%26serial_value%3D%26ver-1%3Dtrue%26ver-3%3Dfalse"
> +        "%26caChoiceradio-SignWithDefaultkey%3Dtrue%26caChoiceradio-SignWithRandomChain%3Dfalse"
> +        "%26autoCAs%3D%26caChoiceradio-SignWithSpecifiedChain%3Dfalse%26manCAs%3D%26%24";

This is still very long.  Why not add a hard break at line 79?

Comment 12

[Kai Engert (:KaiE:)]

Attachment: 1254918-c7-v2.patch

I was lazy and thought less line breaks is less work, and then I thought, maybe it's nice to keep the url paramters together.

Anyway, I've now adjusted the patch to break at 79, regardless of position.

Comment 13

[Kai Engert (:KaiE:)]

(In reply to Kai Engert (:kaie) from comment #12)
> Created attachment 8744096 [details] [diff] [review]
> 1254918-c7-v2.patch

https://hg.mozilla.org/projects/nss/rev/94d9c57057ac