Closed Bug 1254918 Opened 5 years ago Closed 5 years ago

clang-format NSS: cmd

Categories

(NSS :: Libraries, defect)

defect
Not set
normal

Tracking

(firefox48 affected)

RESOLVED FIXED
Tracking Status
firefox48 --- affected

People

(Reporter: mt, Assigned: franziskus)

References

(Blocks 1 open bug)

Details

Attachments

(3 files, 1 obsolete file)

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.
Assignee: nobody → franziskuskiefer
Summary: clang-format NSS: cmd/bltest and cmd/vfychain → clang-format NSS: cmd
Attached patch CmdClangFormat.patch (obsolete) — Splinter Review
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.
Attachment #8730311 - Flags: review?(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?
Attachment #8730311 - Flags: review?(kaie) → review-
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?
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
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.
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.
Attachment #8730311 - Attachment is obsolete: true
Attachment #8733453 - Flags: review?(kaie)
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 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.
Attachment #8733453 - Flags: review?(kaie) → review+
Keywords: checkin-needed
Target Milestone: --- → 3.24
(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
Keywords: checkin-needed
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?
Attachment #8744026 - Flags: review?(martin.thomson) → review+
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.
(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
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.