Closed Bug 1463266 Opened 2 years ago Closed 3 months ago

Code clean-up: Style nits, typos and trailing spaces - take 3

Categories

(Thunderbird :: General, enhancement)

enhancement
Not set
normal

Tracking

(thunderbird78 fixed)

RESOLVED FIXED
Thunderbird 74.0
Tracking Status
thunderbird78 --- fixed

People

(Reporter: jorgk-bmo, Unassigned)

Details

Attachments

(4 files, 2 obsolete files)

+++ This bug was initially created as a clone of Bug #1399756 +++
+++ This bug was initially created as a clone of Bug #1393219 +++

To facilitate continuous integration, I will collect some patches here that can be landed if no other patch is available to trigger a build.
Keywords: leave-open
Attached patch 1463266-mimei.patch (obsolete) — Splinter Review
Attachment #8979395 - Flags: review?(acelists)
Attached patch 1463266-mimei-ignore-ws.patch (obsolete) — Splinter Review
Here a |hg qdiff -w| for easier review.
Comment on attachment 8979395 [details] [diff] [review]
1463266-mimei.patch

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

Thanks for the cleanup.

::: mailnews/mime/src/mimei.cpp
@@ +563,5 @@
>              // place to put the define. The others seems to be in net.h
>              // but is that really really the right place? There is also
>              // a nsMimeTypes.h but that one isn't included. Bug?
> +            char *content_type_format = content_type_row ?
> +               MimeHeaders_get_parameter(content_type_row, "format", NULL,NULL) : 0;

Missing space? Also, can we use nullptr instead of the NULLs? I think BenB wrote in the email the file is parsed as C++ already (and has .cpp extension) so maybe it would work? It already uses 'false/true' bools which I think are not in plain C (may be in some new C versions).

@@ +1048,2 @@
>    else if (!child->superclass)
> +    return false;

Isn't it possible to drop this 'else' too?

@@ +1174,1 @@
>    else if (clearsigned_counts &&

Isn't it possible to drop this 'else' too?

@@ +1184,5 @@
>   */
>  bool
>  mime_crypto_stamped_p(MimeObject *obj)
>  {
>    if (!obj) return false;

This could be wrapped.

@@ +1231,5 @@
>  
>      if (part_begin)
>      {
>        for (; (*s && *s != '?' && *s != '&'); s++)
> +        ;

What? :) Maybe making this a 'while' with the body being 's++' would be more readable.

@@ +1398,5 @@
> +      obj->parent &&
> +      obj->parent->headers &&
> +      mime_typep(obj->parent,
> +                 (MimeObjectClass *)&mimeMultipartAppleDoubleClass))
> +    result = MimeHeaders_get_name(obj->parent->headers, obj->options);

Please add {} so the body is more explicit.
It is already better than the original, without indent it was questionable whether the 'result =' even belonged to the if().

@@ +1403,5 @@
>  
>    /* Else, if this part is itself an AppleDouble, and one of its children
>     has a name, then use that (check data fork first, then resource.) */
>    if (!result &&
> +    mime_typep(obj, (MimeObjectClass *)&mimeMultipartAppleDoubleClass))

More indenting needed to align below !result ?

@@ +1413,1 @@
>      result = MimeHeaders_get_name(cont->children[1]->headers, obj->options);

Indent here?

@@ +1419,1 @@
>      result = MimeHeaders_get_name(cont->children[0]->headers, obj->options);

Indent here?

@@ +1489,1 @@
>    if (! q) return 0;

Maybe drop the space?

@@ +1493,5 @@
>      const char *end, *value, *name_end;
>      for (end = q; *end && *end != '&'; end++)
>        ;
>      for (value = q; *value != '=' && value < end; value++)
>        ;

More candidates for a 'while'.

@@ +1571,4 @@
>     busted: here's a comparison of the old and new numberings with a pair
>     of hypothetical messages (one with a single part, and one with nested
>     containers.)
>     NEW:      OLD:  OR:

Does this block also need aligning with the block below?

@@ +1584,5 @@
> +  message/rfc822        1.4       -     4
> +  multipart/mixed       1.4.1     4     -
> +  text/plain            1.4.1.1   4.1   -
> +  image/jpeg            1.4.1.2   4.2   -
> +  text/plain            1.5       5     5

Nice.

@@ +1854,5 @@
>    const char *s = strrchr(url, '?');
>    if (s && !strncmp(s, "?type=application/x-message-display", sizeof("?type=application/x-message-display") - 1))
>    {
>      const char *nextTerm = strchr(s, '&');
> +    s = nextTerm ? nextTerm : s + strlen(s) - 1;

Is this meant to be 's = nextTerm ? nextTerm : (s + strlen(s) - 1);' or '(nextTerm ? nextTerm : s) + strlen(s) - 1;' ? I'd add the explicit ().
Attachment #8979395 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0c20e0da5cb8
replace tabs with spaces in db/. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/afa140bcba9f
replace tabs with spaces in mailnews/base. rs=white-space-only
This must be some of our ugliest code. I'm not even sure whether mailnews/base/src/nsSubscribeDataSource.cpp is still used.

For me the winner was:
https://hg.mozilla.org/comm-central/rev/afa140bcba9f#l1.238
-/* B1AA0820-D04B-11d2-8069-006008128C4E */
 #define NS_MSGSTATUSFEEDBACK_CID \
-{ 0xbd85a417, 0x5433, 0x11d3, \
-  {0x8a, 0xc5, 0x0, 0x60, 0xb0, 0xfc, 0x4, 0xd2} }
+{ 0xbd85a417, 0x5433, 0x11d3,    \
+{ 0x8a, 0xc5, 0x0, 0x60, 0xb0, 0xfc, 0x4, 0xd2 } }

So a needless comment repeating the numbers which follow, and the comment was wrong :-(

More tabs in mailnews/addrbook, mailnews/compose, mailnews/db, mailnews/extensions, mailnews/imap, mailnews/mapi, mailnews/mime and mailnews/news.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a2c9c8bdb42b
fix inconsistent closing brackets in nsMsgBaseCID.h. rs=white-space-only DONTBUILD
The plan is to kill nsSubscribeDataSource.cpp in the subscribe rewrite bug.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7692c0e19b39
replace tabs with spaces in mailnews/addrbook. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7148c6f9fce2
replace tabs with spaces in mailnews/extensions+mapi+mime+news. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1c587b61bc8b
replace tabs with spaces in mailnews/addrbook+base+extensions+import+intl+local+test. rs=white-space-only
There will be tree more patches in mailnews: compose, db, imap.
There are some tabs in mail, some in editor, and an awful lot in ldap.
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/4586a5565e52
replace tabs with spaces in editor/. rs=white-space-only
https://hg.mozilla.org/comm-central/rev/7b16aeb90747
replace tabs with spaces in mail/. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/53797651df65
replace tabs with spaces in mailnews/db. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/78f99b05889a
replace tabs with spaces in mail/. rs=white-space-only DONTBUILD
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1940b43f2306
replace tabs with spaces in mailnews/compose. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/a6474bd264e6
replace tabs with spaces in mailnews/imap. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3d4271853049
replace tabs with spaces in mailnews/imap (follow-up). rs=white-space-only
https://hg.mozilla.org/comm-central/rev/4b27cf3e7225
replace tabs with spaces in comm-central in IDL files. rs=white-space-only
Addressed review comments.

https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=02eb547cbf26bc2974a8bacd82e587ce8a955263
Assignee: nobody → jorgk
Attachment #8979395 - Attachment is obsolete: true
Attachment #8979396 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8985421 - Flags: review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/01215911a27e
code clean-up in mimei.cpp. r=aceman
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/5ebc1943af38
replace tabs with spaces in ldap/. rs=white-space-only
afaik ldap contains an upstream library, did we really want to diverge from it substantially by converting tabs/spaces?
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/829b7ff18e5b
Follow-up: aligned constants in ldaprot.h. rs=white-space-only DONTBUILD
(In reply to Philipp Kewisch [:Fallen]  from comment #22)
> afaik ldap contains an upstream library, did we really want to diverge from
> it substantially by converting tabs/spaces?
Good question. I think the version we have is really old and if we ever did something to LDAP, we'd replace it completely. If we'd ever have to compare the current version with a new one, "-w" will take care, don't you think? Also, chances are the the new version also corrected white-space issues.

Tabs are a real pain, especially in LDAP where they could have meant 1, 2, 4 or 8 spaces, so I'm glad they're gone now.
I needed to remove a few tabs for another patch to apply:
https://hg.mozilla.org/releases/comm-beta/rev/be58f31115b63d29b5ceef414bb2447c4c138903 (BETA_60_CONTINUATION)
replace tabs with spaces in nsSubscribableServer.cpp. rs=white-space-only a=jorgk DONTBUILD
(In reply to Pulsebot from comment #20)
> Pushed by mozilla@jorgk.com:
> https://hg.mozilla.org/comm-central/rev/01215911a27e
> code clean-up in mimei.cpp. r=aceman
Uplifted that to beta so other patches can apply:
https://hg.mozilla.org/releases/comm-beta/rev/a195e8eaa188c57c067a2d8106ba6f4e3e227f96 - BETA_60_CONTINUATION
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/cfc93bec7d61
Unify spelling of 'normalized'. rs=comment-only,typo-fix
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1e38647496ae
fix some white-space issues in nsAddrDatabase.cpp. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/accc44126f08
fix another white-space issue in nsAddrDatabase.cpp. rs=white-space-only
It's not really clear what the difference is, but PromiseFlat[C]String is used in all other cases, so let's get rid of these two here.
Attachment #9010156 - Flags: review?(acelists)
Comment on attachment 9010156 [details] [diff] [review]
1463266-nsPromiseFlat.patch

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

Seems to me nsPromiseFlatString is a type, so the occurences you found look like a cast.
While PromiseFlatString() may be a function that returns a safe copy of the string.

There are only 3 occurences of nsPromiseFlatString() use in m-c in https://dxr.mozilla.org/comm-central/source/dom/base/ImageEncoder.cpp, so yes, it looks proper to convert to PromiseFlatString().
Attachment #9010156 - Flags: review?(acelists) → review+
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/ca9d4cde9481
Change nsPromiseFlat[C]String to PromiseFlat[C]String for consistency. r=aceman
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/86c4629ad6e4
fix white-space/newline issue in calendar-unifinder-todo.xul. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/00ddcafd807a
Fix white-space issue in nsMsgBodyHandler::StripHtml(). rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/3be6410d75a4
fix yet another white-space issue in nsAddrDatabase.cpp. rs=white-space-only DONTBUILD
Cleanup in mailnews/base/content/dateFormat.js
Attachment #9030112 - Flags: review?(jorgk)
Comment on attachment 9030112 [details] [diff] [review]
1463266-dateFormat.js

Thanks.
Attachment #9030112 - Flags: review?(jorgk) → review+
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/9e7cf2d56e15
clean up white-space in mailnews/base/content/dateFormat.js. r=jorgk
Keywords: checkin-needed
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/14af8a6f714b
remove unneeded namespace qualifier in nsMsgCompose.cpp. r=me

https://hg.mozilla.org/releases/comm-esr60/rev/514b38608a948c783d0e2fc26b75c6283b2eb36a
replace tabs with spaces in comm-central in IDL files. rs=white-space-only

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6d5f99e38a3f
remove \n in NS_ASSERTION(). r=me
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/708086efbb2c
fix white-space issue/indentation in comment in nsMsgAttachmentHandler.cpp. rs=comment-only DONTBUILD
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/bbc858a8e20d
remove trailing spaces in ldap. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0db6d6cc4f92
fix typos in mailnews/ using codespell. rs=comment-only,typo-fix
https://hg.mozilla.org/comm-central/rev/73ecaa37ed5b
fix typos in mail/ using codespell. rs=comment-only,typo-fix
https://hg.mozilla.org/comm-central/rev/599d4d27b9ff
fix typos in ldap/ using codespell. rs=comment-only,typo-fix
https://hg.mozilla.org/comm-central/rev/18cc85ecd842
fix typos in chat/, common/ and editor/ using codespell. rs=comment-only,typo-fix
https://hg.mozilla.org/comm-central/rev/a90d5a8ce30d
fix typos in db/ using codespell. rs=comment-only,typo-fix DONTBUILD
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/6d861eaffbe9
Fix wrong indentation in Windows8WindowFrameColor.jsm. rs=white-space-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/93224a560c9c
Fix some spelling mistakes. rs=comment-only,typo-fix DONTBUILD
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0532fb6e8fa3
Fix comments in mimetext.cpp. rs=comment-only DONTBUILD
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7f8bd17d9565
remove trailing spaces from IDL files. rs=white-space-only DONTBUILD
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/8448cd61c29f
Fix/remove some ugly/unneeded comments in nsMsgCompose.cpp. rs=comment-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/2c82043b716d
Fix some ugly comments in nsMsgAttachment*.cpp. rs=comment-only
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fbf344af33cf
Fix some ugly comments in nsMsgCompUtils.cpp. rs=comment-only DONTBUILD
Assignee: jorgk → nobody
Status: ASSIGNED → NEW
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/328093467a65
remove \n in MOZ_LOG and NS_ERROR. r=me DONTBUILD
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/7d63ed41d20c
remove \n in MOZ_LOG and NS_ERROR. r=me DONTBUILD
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/0d0c445e5077
remove \n in MOZ_LOG. r=me
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/1edda72532b4
Remove space before function arguments in IDL files. rs=white-space-only DONTBUILD
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e4d9fac14a30
fix typos in comm-central using codespell. rs=comment-only,typo-fix
Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/e5ccd3ff3f18
fix typos in comm-central using codespell. rs=comment-only,typo-fix

Some trivial stuff to land when needed :)

Attachment #9118821 - Flags: review?(geoff)
Attachment #9118821 - Flags: review?(geoff) → review+

Pushed by mozilla@jorgk.com:
https://hg.mozilla.org/comm-central/rev/fd13b1c3b79c
Fix some typos in MailMigrator.jsm. r=darktrojan

Status: NEW → RESOLVED
Closed: 3 months ago
Keywords: leave-open
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 75.0
Target Milestone: Thunderbird 75.0 → Thunderbird 74.0
You need to log in before you can comment on or make changes to this bug.