1232306 - Far too many compiler warnings

Status: RESOLVED FIXED

(Thunderbird :: General, defect)

Description

[Lars Rohwedder]

User Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10_11_2) AppleWebKit/601.3.9 (KHTML, like Gecko) Version/9.0.2 Safari/601.3.9

Steps to reproduce:

I compile Thunderbird and got a lot of compiler warnings. Even if most of them will not result in user-visible bugs or crashes I'd like to fix them anyway to keep the code clean and avoid defocusing. 


Actual results:

The warnings fall into these categories:
1.  array subscript is of type 'char'
2.  XX overrides a member function but is not marked 'override' [-Winconsistent-missing-override]
3.  ISO C++11 does not allow conversion from string literal to 'char *' [-Wwritable-strings]
4. variable X is used uninitialized whenever 'if' condition is false


Expected results:

The code should be compile without any warnings.
In this item I'll focus on the Thunderbird code only. :-)

Comment 1

[Lars Rohwedder]

There is another compiler warning that seems to reveal a more serious bug:

Warning: -Wstring-plus-char in /Users/lars/hg/comm-central/mailnews/local/src/nsLocalMailFolder.cpp: adding 'char' to a string pointer does not append to the string

/Users/lars/hg/comm-central/mailnews/local/src/nsLocalMailFolder.cpp:2204:36: warning: adding 'char' to a
 string pointer does not append to the string [-Wstring-plus-char]

         memcpy (end, MSG_LINEBREAK + '\0', MSG_LINEBREAK_LEN + 1);
                      ~~~~~~~~~~~~~~^~~~~~

I guess the author wanted to write that:

         memcpy (end, MSG_LINEBREAK "\0", MSG_LINEBREAK_LEN + 1);

Comment 2

[Lars Rohwedder]

Attachment: Bug_1232306_add_override.patch

This patch adds the missing "override" keyword to source files in mail/ and mail news/ , incl. some code format cleanups I could not resist to do also.

Comment 3

[Lars Rohwedder]

Attachment: Bug_1232306_const_string_literals.patch

This patch fixes the compiler warnings about "ISO C++11 does not allow conversion from string literal to 'char *' [-Wwritable-strings]"

Comment 4

[Jorg K (CEST = GMT+2)]

I think the coding convention is abort() and not abort ().

Comment 5

[Lars Rohwedder]

(In reply to Jorg K (GMT+1) from comment #4)
> I think the coding convention is abort() and not abort ().

That space between abort and () didn't came from me. It looks like generated code from yacc. I just changed the indentation.

Comment 6

[Jorg K (CEST = GMT+2)]

(In reply to Lars Rohwedder from comment #5)
> I just changed the indentation.
I think the rule is that whatever gets touched gets fixed ;-) So if you fix the indentation, please remove the space as well. Doesn't have to happen now, do it after the review.

Comment 7

[Lars Rohwedder]

(In reply to Jorg K (GMT+1) from comment #6)
> (In reply to Lars Rohwedder from comment #5)
> > I just changed the indentation.
> I think the rule is that whatever gets touched gets fixed ;-) So if you fix
> the indentation, please remove the space as well. Doesn't have to happen
> now, do it after the review.

Okay, got it. 

Well, there are two ways of coding:

1) focus on your issue! fix _only_ that things that your issue describes/demands and leave the rest untouched, so the changes are as small as possible (and maybe you might open another bug ticket specific to the issues you found in the code)

2) follow the "Boy Scout Rule" and leave the code you're working on in a better state than it was before.

I don't know which way is preferred on Mozilla, so I tried to keep my "impact" on the code quite small.

If Mozilla prefer the second way: How wide this "code cleanup area" is allowed to be: some lines, 1 function, 1 src file?

 http://programmer.97things.oreilly.com/wiki/index.php/The_Boy_Scout_Rule

Comment 8

[Jorg K (CEST = GMT+2)]

Hmm, it's a matter of judgement. Sometimes I leave bad stuff alone, that is "minimal change". But sometimes I fix bad stuff so code becomes more readable.

In your case, there is no doubt. The patches are line based, not character based. Line removed, line added:
-  abort ();
+    abort ();

This should be:
-  abort ();
+    abort();

You don't need any judgement for that.

Comment 9

[Lars Rohwedder]

> In your case, there is no doubt. The patches are line based, not character
> based. Line removed, line added. […] You don't need any judgement for that.

Okay. :-)

Comment 10

[Magnus Melin [:mkmelin]]

Don't forget to ask for reviews for the patches, or else they risk being left to rot.

Comment 11

[Lars Rohwedder]

(In reply to Magnus Melin from comment #10)
> Don't forget to ask for reviews for the patches, or else they risk being
> left to rot.

Oh yes, thanks. But... who shall I ask and how?

Comment 12

[Magnus Melin [:mkmelin]]

Try jcranmer, neil or rkent. https://wiki.mozilla.org/Modules/All#MailNews_Core

As for how, set the "review?" flag on the attachment to their email address. See https://developer.mozilla.org/en-US/docs/Mozilla/Developer_guide/How_to_Submit_a_Patch#Getting_the_patch_reviewed

Comment 13

[Lars Rohwedder]

Attachment: Bug_1232306_fix_char_index.patch

mini-patch to avoid char literals as array index. Its use is safe here but the compiler complains, so cast the literals to uint8_t and we can focus on more important warnings :-)

Comment 14

[Lars Rohwedder]

Attachment: Bug_1232306_initialize_local_variables.patch

Comment 15

[Joshua Cranmer [:jcranmer]]

(In reply to Lars Rohwedder from comment #7)
> I don't know which way is preferred on Mozilla, so I tried to keep my
> "impact" on the code quite small.
> 
> If Mozilla prefer the second way: How wide this "code cleanup area" is
> allowed to be: some lines, 1 function, 1 src file?

The rule of thumb is that you fix all the style nits in any line you change, and sometimes immediately adjacent lines. It depends on how problematic the style nits are; for things like "bad indentation", you generally fix the entire block/function if you're changing enough of it.

Comment 16

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8698578 [details] [diff] [review]
Bug_1232306_add_override.patch

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

r-.

This patch is unreviewable; you appear to have literally concatenated several patches into a single file before uploading, which renders it impossible to review via the UI. I got through as much as I could review (which wasn't much).

At the very least, the commit message is patently incorrect: for a patch that claims to be doing nothing but adding overrides, it's clearly doing *far* more.

::: mailnews/import/src/nsImportAddressBooks.cpp
@@ +54,2 @@
>  
> +  NS_IMETHOD BeginImport(nsISupportsString *successLog, nsISupportsString *errorLog, bool *_retval) override;

Speaking of style nits, we use 80-character lines as much as possible.

@@ +71,5 @@
>    static void ReportError(const char16_t *pName, nsString *pStream,
>                            nsIStringBundle *aBundle);
>  
>  private:
> +  nsIImportAddressBooks*      m_pInterface = nullptr;

And we tend to prefer T *foo;

In particular, in this block, we don't align variable definition.

Also, while this is legal C++11 and it looks like all the compilers support this, I'd prefer it if you initialize these variables not in-line in the class definition but in the constructor initializer.

@@ +92,5 @@
>  };
>  
>  class AddressThreadData {
>  public:
> +  bool              driverAlive  = false;

Ditto.

::: mailnews/import/eudora/src/nsEudoraCompose.cpp
@@ +119,4 @@
>  
>    /* void OnStopSending (in string aMsgID, in nsresult aStatus, in wstring aMsg, in nsIFile returnFile); */
>    NS_IMETHOD OnStopSending(const char *aMsgID, nsresult aStatus, const char16_t *aMsg,
> +               nsIFile *returnFile) override 

The style here would be to align the nsIFile with the const char above.

And lose the extraneous trailing whitespace.

@@ +142,5 @@
>  
>    void Reset() { m_done = false;  m_location = nullptr;}
>  
>  public:
> +  bool m_done = false;

See above notes.

::: mailnews/import/src/nsImportMail.cpp
@@ +54,5 @@
>  
>    NS_DECL_THREADSAFE_ISUPPORTS
>  
>    /* nsISupports GetData (in string dataId); */
> +  NS_IMETHOD GetData(const char *dataId, nsISupports **_retval) override;

Ditto about alignment.

Comment 17

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8698582 [details] [diff] [review]
Bug_1232306_const_string_literals.patch

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

r- for the same reason as above: I see nsMsgKeySet repeated twice, which means I don't trust that the patch file was constructed properly.

::: mailnews/import/eudora/src/nsEudoraEditor.cpp
@@ +11,5 @@
>  #include "nsMsgUtils.h"
>  #include "nsNetUtil.h"
>  #include "nsImportEmbeddedImageData.h"
>  
> +static const char *     sEudoraEmbeddedContentLines[] = {

Again, too much whitespace here.

::: mailnews/compose/src/nsMsgAppleEncode.cpp
@@ +362,5 @@
>  
>  /* Description of the various file formats and their magic numbers 		*/
>  struct magic 
>  {
> +    const char* const name;			/* Name of the file format 					*/

Also, change these to // C++ style comments, and drop the alignment.

@@ +375,5 @@
>      { "image/jpeg", "\377\330\377",   0 },
>      { "video/mpeg", "\0\0\001\263",	  4 },
>      { "application/postscript", "%!", 0 },
>  };
> +static const int num_magic = (sizeof(magic)/sizeof(magic[0]));

Import mozilla/ArrayLength.h and use MOZ_ARRAY_LENGTH(magic) here.

@@ +380,2 @@
>  
> +static const char * const text_type    = TEXT_PLAIN; /* the text file type.	*/		

Trailing white space.

@@ +398,5 @@
>      {
>  		if (numread >= magic[i].len) 
>  		{
> +			int j=0;
> +	    	for (; j<magic[i].len; j++) 

The indentation is wrong, which makes me suspect you're using tabs here. DO NOT USE TABS, EVER (unless you're in a Makefile, because Make sucks).

Comment 18

[Joshua Cranmer [:jcranmer]]

Comment on attachment 8700662 [details] [diff] [review]
Bug_1232306_fix_char_index.patch

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

It boggles my mind that a compiler would warn on this code. While the signedness of char may vary on different systems, the char literals are all clearly in the range of [0, 128), which is legal independent of the sign on all systems.

Comment 19

[Kent James (:rkent)]

Comment on attachment 8700686 [details] [diff] [review]
Bug_1232306_initialize_local_variables.patch

So an initial look at this patches shows that you are giving default values to all variables, regardless of whether that variable can ever be used with a default value. I'm looking for example at:

-    const char *charset;
+    const char *charset = nullptr;

where the first time we encounter this variable, it is set:

charset = msgCharset.get();

Is that your intention? Did this generate a compile warning, and if so where and why? That usage seems valid to me without your proposed change.

Comment 20

[Lars Rohwedder]

> This patch is unreviewable; you appear to have literally concatenated
> several patches into a single file before uploading, which renders it
> impossible to review via the UI. I got through as much as I could review
> (which wasn't much).
> 
> At the very least, the commit message is patently incorrect: for a patch
> that claims to be doing nothing but adding overrides, it's clearly doing
> *far* more.

Okay, my fault: I made many commits into my local repository and than I just run "hg export" that bundles all my commits into one patch file.

Now I know better: For the last 2 patches ("char literals" and "uninitialised variables" I did it in only one commit, incl. proper commit message (I hope so).

Are the last 2 patches better (in matters of commit guidelines, not the content/code it covers)?

> >  private:
> > +  nsIImportAddressBooks*      m_pInterface = nullptr;
> 
> And we tend to prefer T *foo;

Oh, you prefer C style. I didn't know that. I've seen both styles in the code and I prefer C++ style so I used it here, too: http://www.stroustrup.com/bs_faq2.html#whitespace

> In particular, in this block, we don't align variable definition.
> 
> Also, while this is legal C++11 and it looks like all the compilers support
> this, I'd prefer it if you initialize these variables not in-line in the
> class definition but in the constructor initializer.

The problem with constructor initialisers is that the initialization code is far away from the variable declaration so the risk is high that some members are forgotten (and actually: in some classes I've seen there _were_ member variables that are not mentioned in the c'tor initializer list, hence they might not be initialized at all).

And because not all compilers support "delegating constructors" yet, the c'tor initializer list has to be repeated in each constructor. Many duplicated code, error-prone duplication etc. That's why I prefer the new direct initialisation in the header, that – as you mentioned correctly – is supported by all the compilers we need.

> >    NS_IMETHOD OnStopSending(const char *aMsgID, nsresult aStatus, const char16_t *aMsg,
> > +               nsIFile *returnFile) override 
> 
> The style here would be to align the nsIFile with the const char above.

Okay.

Comment 21

[Lars Rohwedder]

> r- for the same reason as above: I see nsMsgKeySet repeated twice, which
> means I don't trust that the patch file was constructed properly.

What is the correct / recommended way to create patch files properly?

I made my changes in a local branch with many local commits (normally one commit per file, except when I've something forgotten, than the file is changed twice) and created the patch file with "hg export" which stores all these local commits in the patch file, which I didn't know and what is of course unnecessary for you as a reviewer or for the final change history.


> >  /* Description of the various file formats and their magic numbers 		*/
> >  struct magic 
> >  {
> > +    const char* const name;			/* Name of the file format 					*/
> 
> Also, change these to // C++ style comments, and drop the alignment.

Okay, so the Boy's Scout rule applies also here: If I change code and see C-style comments I should change them to C++-style comments. I'll do. :-)

> @@ +375,5 @@
> >      { "image/jpeg", "\377\330\377",   0 },
> >      { "video/mpeg", "\0\0\001\263",	  4 },
> >      { "application/postscript", "%!", 0 },
> >  };
> > +static const int num_magic = (sizeof(magic)/sizeof(magic[0]));
> 
> Import mozilla/ArrayLength.h and use MOZ_ARRAY_LENGTH(magic) here.

I didn't know this macro.  But if I'm refactoring this code already, should I change it into a "static const std::vector<>" instead of a C-style array? What do you think?

> @@ +398,5 @@
> >      {
> >  		if (numread >= magic[i].len) 
> >  		{
> > +			int j=0;
> > +	    	for (; j<magic[i].len; j++) 
> 
> The indentation is wrong, which makes me suspect you're using tabs here. DO
> NOT USE TABS, EVER (unless you're in a Makefile, because Make sucks).

Okay.

Comment 22

[Lars Rohwedder]

Attachment: Bug_1232306_fix_string_literals_2.patch

2nd try to fix the string literal constness issue.
* I changed the C comments into C++ comments in the files I touched.
* I removed the space between "abort" and "()"
* removed deprecated "register" keyword.

Comment 23

[Jorg K (CEST = GMT+2)]

Comment on attachment 8705281 [details] [diff] [review]
Bug_1232306_fix_string_literals_2.patch

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

Lot's of white-space issues here, sorry, we're very picky ;-)
Formally I can only review under mail/components/compose, sorry, very strict rules here.
Other than that it looks OK, although I ask myself why you bothered to change the comments. The idea was to fix compiler warnings, right? Yes, I know, "boy scout" rules, but here I can't see the real change for all the comment changes.
Another comment would be that Eudora is DEAD(!!). I'd much rather see all the Eudora code removed than fixed.

::: mailnews/base/util/nsMsgKeySet.cpp
@@ +226,5 @@
>  {
>    if (m_length <= 0) {
>      return 1;
>    } else if(m_data[0] < 0 && m_data[1] != 1 && m_data[1] != 0) {
> +    // first range not equal to 0 or 1, always return 1 

Trailing space, here an in many more occasions.

@@ +231,3 @@
>      return 1;
>    } else if (m_data[0] < 0) {
> +    // it's a range 

here.

@@ +235,3 @@
>      return (m_data[1] - m_data[0] + 1);
>    } else {
> +    // it's a literal 

here.

@@ +238,2 @@
>      if (m_data[0] == 1) {
> +      // handle "1,..." 

here ... you got the message ;-)
Before you submit a patch please look for " $" and make sure that no added lines have trailing spaces.
And while you're in cleanup mode, you might want to remove some pre-existing trailing spaces as well.
If you're wondering how the reviewers spot the trailing spaces, try to do a review of your own patch by clicking on "Review". You'll be surprised.

::: mailnews/local/src/nsLocalMailFolder.cpp
@@ +2200,5 @@
>        // to append the LINEBREAK to the buffer to enable transfer of the last line.
>        if (mCopyState->m_wholeMsgInStream)
>        {
>          end = start + mCopyState->m_leftOver;
> +        memcpy (end, MSG_LINEBREAK "\0", MSG_LINEBREAK_LEN + 1);

Please remove space after function name.
Looks this is defined as:
#define MSG_LINEBREAK "\015\012"
#define MSG_LINEBREAK_LEN 2
(depending on the platform)
Can you please enlighten me what the + '\0' used to do.

Comment 24

[Kent James (:rkent)]

Comment on attachment 8705281 [details] [diff] [review]
Bug_1232306_fix_string_literals_2.patch

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

Grrr, I had some comments that got lost, probably a collision with Jorgk.

I'm not going to repeat them. Jorgk's comments are fine.

As I said in IRC, way too many cosmetic changes outside of immediate lines that were changed. Please do not in the future combine massive cosmetic changes with a few lines of non-cosmetic changes.

Overall, I think such cosmetic changes are a waste of time except on the few lines of code that you actually change.

I can;t really check the removal of warnings easily. Could you attach here a log output showing the warnings before and after your changes, so that I can see why you did these changes?

I would be happy for Jorg to do the detailed review with my rs if he is comfortable with that.

Comment 25

[Jorg K (CEST = GMT+2)]

(In reply to Kent James (:rkent) from comment #24)
> As I said in IRC, way too many cosmetic changes outside of immediate lines
> that were changed. Please do not in the future combine massive cosmetic
> changes with a few lines of non-cosmetic changes.
> 
> Overall, I think such cosmetic changes are a waste of time except on the few
> lines of code that you actually change.
I agree. Could we have the patch with minimal change, that is, only change the lines necessary to be changed to get rid of the warnings.

> I would be happy for Jorg to do the detailed review with my rs if he is
> comfortable with that.
OK.

Comment 26

[Kent James (:rkent)]

Comment on attachment 8700662 [details] [diff] [review]
Bug_1232306_fix_char_index.patch

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

While I agree with comment 18, the risk of this is very low and it is not really worth a discussion about in what cases this might be needed. Let's just give Roker the benefit of the doubt, and land this without any more effort on it.

Comment 27

[Lars Rohwedder]

> Lot's of white-space issues here, sorry, we're very picky ;-)

No problem. That what review is for. And I am still learning. :-)

> Formally I can only review under mail/components/compose, sorry, very strict
> rules here.

So I shall split the changes into many patches, one patch per component?

> Other than that it looks OK, although I ask myself why you bothered to
> change the comments.

Because it was told me some comments above to change the C comments into C++ comments. It was not my idea.

> The idea was to fix compiler warnings, right? Yes, I
> know, "boy scout" rules, but here I can't see the real change for all the
> comment changes.

Hence I ask for a "radius" how far the boy scout shall swarm for de-littering and clean-up. :jcranmer suggest to fix/clean-up the function I am just working on.

But there are several quite similar functions in the same file and it looked odd to me if I tidy up just one of them and leave all the others as they are. So I changed them, too. And so 80% of the code is touched, it did not matter to change the remaining 20%, too.

:-/

> Another comment would be that Eudora is DEAD(!!). I'd much rather see all
> the Eudora code removed than fixed.

Okay, where is the ticket that removes all that code? :-)

> Before you submit a patch please look for " $" and make sure that no added
> lines have trailing spaces.
> And while you're in cleanup mode, you might want to remove some pre-existing
> trailing spaces as well.

Normally I do. So I wonder why I haven't seen them. My fault.

> ::: mailnews/local/src/nsLocalMailFolder.cpp
> @@ +2200,5 @@
> >        // to append the LINEBREAK to the buffer to enable transfer of the last line.
> >        if (mCopyState->m_wholeMsgInStream)
> >        {
> >          end = start + mCopyState->m_leftOver;
> > +        memcpy (end, MSG_LINEBREAK "\0", MSG_LINEBREAK_LEN + 1);
> 
> Please remove space after function name.

Okay.

> Looks this is defined as:
> #define MSG_LINEBREAK "\015\012"
> #define MSG_LINEBREAK_LEN 2
> (depending on the platform)
> Can you please enlighten me what the + '\0' used to do.

The old code just didn't make any sense. "xx" + '\0' is the same as "xx" in C and C++, because it is the addition of a pointer (pointing to the string literal) and the number 0 (where the NUL character literal is promoted tp, automatically). It is _not_ a string concatenation, though.

But, because the length term contained a +1, I guessed the author of the code wanted a string concatenation. This is easy in C and C++ if both parts are string literals: "xx" "\0" is just the same as "xx\0". So I changed the code that way.

Comment 28

[Lars Rohwedder]

Attachment: Bug_1232306_add_override-2.patch

Okay, here is my second approach to add the missing "override" and mute these compiler warnings. Review from _one_ of you is sufficient. 

What is done also in this patch ("boy scout rule", I hope the radius of these changes was not too big):

* changed single line C-style comments into C++ style comments
* unify the coding style of some classes ("*" goes to the type, as desired by Mozilla Coding Style Guide)
* use less error-prone member initialisation which makes self-written default c'tors (that forgot some members to initialise!) and d'tor obsolete.

Comment 29

[Jorg K (CEST = GMT+2)]

(In reply to Lars Rohwedder from comment #27)
> So I shall split the changes into many patches, one patch per component?
I think, it's OK as you've done now since it's just a bunch of little changes which don't change the behaviour.
 
> Hence I ask for a "radius" how far the boy scout shall swarm for
> de-littering and clean-up. :jcranmer suggest to fix/clean-up the function I
> am just working on.
Now it's good, the reviewer can concentrate on the code changes. It really a matter of judgement. If you only work on one function and do a lot of changes, then you might as well clean up the rest of that function as well. But for a one line change, I wouldn't touch the rest. 

> > Before you submit a patch please look for " $" and make sure that no added
> > lines have trailing spaces.
> Normally I do. So I wonder why I haven't seen them. My fault.
Still some trailing spaces you added, click on "Review" to see them in red/pink.

> The old code just didn't make any sense. "xx" + '\0' is the same as "xx" in
> C and C++, because it is the addition of a pointer (pointing to the string
> literal) and the number 0 (where the NUL character literal is promoted tp,
> automatically). It is _not_ a string concatenation, though.
It didn't make any sense to me either, that's why I asked.

> But, because the length term contained a +1, I guessed the author of the
> code wanted a string concatenation. This is easy in C and C++ if both parts
> are string literals: "xx" "\0" is just the same as "xx\0". So I changed the
> code that way.
I wonder how that ever worked. It moved n+1 bytes and the last byte moved had random content.

Comment 30

[Lars Rohwedder]

(In reply to Jorg K (GMT+1) from comment #29)
> > But, because the length term contained a +1, I guessed the author of the
> > code wanted a string concatenation. This is easy in C and C++ if both parts
> > are string literals: "xx" "\0" is just the same as "xx\0". So I changed the
> > code that way.
> I wonder how that ever worked. It moved n+1 bytes and the last byte moved
> had random content.

Well, a string literal in C and C++ always contains a terminating NUL character, so it is on char longer than it appears to be. e.g. the string literal "ab" is an array of 3 chars: {'a', 'b', '\0'}, both in memory and for the compiler (the data type of "ab" is const char[3]).

So strictly speaking: Adding yet another NUL char to the "\r\n" string literal is not necessary here to avoid a buffer overflow, but I'd add it anyway to express the intention of the programmer explicitly: The first NUL char belongs to the written payload.

Comment 31

[Kent James (:rkent)]

Comment on attachment 8706869 [details] [diff] [review]
Bug_1232306_add_override-2.patch

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

I'm going to r- this since it fails to compile. I spent about an hour looking over it, but only got partway through.

A lot of this patch seems to me to be introducing your own personal C++ style into our codebase. That includes 1) initializing all class member variables, 2) using in-class initializers, 3) adding additional blank lines between member definitions, 4) using the ClassConstructor() = default; construction.

In the process, there is a lot of code churn, and asking me to lookup C++ constructs that are not typical in our code base. I am not convinced this is a good use of review time.

The adding of the overrides keyword is now standard practice in Mozilla code, so a big patch that did nothing but add that would be acceptable.

Patches like this one that try to deal with a lot of code style issues, many of them new to Mozilla code (or at least mailnews code) are really hard to review, and lead to lots of r- and wasted time. Please, keep to one issue in the patches. Give me a patch that just adds override, that would be easy to review and approve.

Before you propose new language features, see if that usage is common in Mozilla code. If not, introduce your proposal in a single patch (like "initialize all member class variables" for example, or "Replace () = {} with () = default") instead of mixing a lot of new issues in a large patch. You'll need to justify why your proposed changes are worthy of being used in both new code, as well as worth the effort to go back and update old code (if that is what you are proposing).

For the issue of () = default, jcranmer did not like that, so you'll need to get his r+ on that one (since he is both resident C++ expert as well as module owner).

::: mailnews/base/src/nsMsgGroupView.h
@@ +41,5 @@
>    NS_IMETHOD OnHdrFlagsChanged(nsIMsgDBHdr *aHdrChanged, uint32_t aOldFlags, 
>                                 uint32_t aNewFlags, nsIDBChangeListener *aInstigator) override;
>  
> +  NS_IMETHOD LoadMessageByViewIndex(nsMsgViewIndex aViewIndex) override;
> +  

Nit: new trailing space

::: mailnews/base/util/nsMsgProtocol.cpp
@@ +1000,5 @@
>  public:
>      // XXX this probably doesn't need to be threadsafe
>      NS_DECL_THREADSAFE_ISUPPORTS
>  
> +    nsMsgProtocolStreamProvider() = default;

Why are you replacing the valid () with default? OK, I see there are a few other uses of this "= default" constructor, but why the code churn to replace existing uses? (I won't repeat this for other uses in the patch, but the same question applies).

::: mailnews/import/text/src/nsTextImport.cpp
@@ +110,3 @@
>    nsCOMPtr<nsIFile> m_fileLoc;
>    nsCOMPtr<nsIStringBundle> m_notProxyBundle;
> +  char16_t m_delim = u'\0';

This line causes a compile error (I use Windows Visual Studio 2013)  I guess this is not allowed: u'\0';

Comment 32

[Jorg K (CEST = GMT+2)]

(In reply to Lars Rohwedder from comment #27)
> > Another comment would be that Eudora is DEAD(!!). I'd much rather see all
> > the Eudora code removed than fixed.
> Okay, where is the ticket that removes all that code? :-)
Here: Bug 1243498.

Comment 33

[Jorg K (CEST = GMT+2)]

(In reply to Lars Rohwedder from comment #13)
> mini-patch to avoid char literals as array index. Its use is safe here but
> the compiler complains, so cast the literals to uint8_t and we can focus on
> more important warnings :-)
Only the Mac compiler complains:
ImportCharSet.cpp:44:25: warning: array subscript is of type 'char' [-Wchar-subscripts]
  ImportCharSet::m_Ascii[')'] |= ImportCharSet::c822SpecialChar;

Anyway, let's land this.

Comment 34

[Jorg K (CEST = GMT+2)]

Comment on attachment 8705281 [details] [diff] [review]
Bug_1232306_fix_string_literals_2.patch

Making this obsolete, so we can move on with this bug. BTW, after Eudora is removed, this is no longer required.

Comment 35

[Jorg K (CEST = GMT+2)]

Comment on attachment 8706869 [details] [diff] [review]
Bug_1232306_add_override-2.patch

Making this obsolete, so we can move on with this bug. BTW, after Eudora is removed, this is no longer required.

Comment 36

[Jorg K (CEST = GMT+2)]

I've landed the little patch:
https://hg.mozilla.org/comm-central/rev/1563ed562ea3

It would be good if you could submit another patch for the more important compiler warnings in another bug. Please follow Kent's comments in comment #31.

Also, please no touching of Eudora import files, as they will be removed in bug 1243498.

Comment 37

[:aceman]

We had to fix all of the compile warnings in bug 1294260 (on all platforms).

The needed fixes comprise many of the ones proposed here (but also new ones), except the style/whitespace changes.

So the warning fixes proposed here got into the tree in the end.