Closed Bug 1232107 Opened 4 years ago Closed 4 years ago

fix some compile warnings in /mailnews

Categories

(MailNews Core :: Testing Infrastructure, defect, trivial)

All
Linux
defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 46.0

People

(Reporter: aceman, Assigned: aceman)

References

Details

Attachments

(1 file, 5 obsolete files)

mailnews/base/util/nsMsgKeySet.cpp: In static member function 'static void nsMsgKeySet::test_member(bool)':
mailnews/base/util/nsMsgKeySet.cpp:1402:5: warning: deprecated conversion from string constant to 'char*' [-Wwrite-strings]
   s = "1-70,72-99,105,107,110-111,117-200";
     ^
mailnews/base/util/nsMsgKeySet.cpp:1414:5: warning: deprecated conversion from string constant to 'char*' [-Wwrite-strings]
   s = "0-70,72-99,105,107,110-111,117-200";

mailnews/imap/src/nsIMAPBodyShell.h: In member function 'nsIMAPMessagePartID* nsIMAPMessagePartIDArray::GetPart(int)':
mailnews/imap/src/nsIMAPBodyShell.h:356:28: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
   NS_ASSERTION(i >= 0 && i < Length(), "invalid message part #");

../../../dist/include/nsError.h:151:40: warning: 'rv' may be used uninitialized in this function [-Wmaybe-uninitialized]
   return static_cast<uint32_t>(aErr) & 0x80000000;
                                        ^
mailnews/base/src/nsMessenger.cpp:2025:12: note: 'rv' was declared here

mailnews/base/src/nsMsgAccountManager.cpp: In member function 'nsresult nsMsgAccountManager::findServerInternal(const nsACString_internal&, const nsACString_internal&, const
 nsACString_internal&, int32_t, bool, nsIMsgIncomingServer**)':
mailnews/base/src/nsMsgAccountManager.cpp:1881:25: warning: unused variable 'foundServer' [-Wunused-variable]

../../../dist/include/nsError.h:151:40: warning: 'rv' may be used uninitialized in this function [-Wmaybe-uninitialized]
   return static_cast<uint32_t>(aErr) & 0x80000000;
                                        ^
mailnews/base/src/nsMsgPrintEngine.cpp:455:12: note: 'rv' was declared here

These should be all current warnings on GCC on Linux, except the "case value 'xxx' not in enumerated type 'nsresult'", which are covered by another bug (merging of c-c error codes into m-c errors).
Attached patch patch (obsolete) — Splinter Review
Try run with no visible problems from this patch: https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=6b8fca257ff3

The patch should be easy to review. The only questionable part is the changing of IMAP message part IDs to be unsigned. Is that a problem? Can partIDs be negative? Or do we need that option for internal purposes?
Attachment #8697780 - Flags: review?(Pidgeot18)
Comment on attachment 8697780 [details] [diff] [review]
patch

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

While you wait for :jcranmer's review, here's a quick drive-by review...

::: mailnews/base/src/nsMessenger.cpp
@@ +2027,5 @@
>  
>    if (!mStringBundle)
>      rv = InitStringBundle();
> +  else
> +    rv = NS_OK;

I'd prefer setting rv to NS_OK when it's declared above.

::: mailnews/base/util/nsMsgKeySet.cpp
@@ +1398,5 @@
>  {
>    nsMsgKeySet *set;
>    char *s;
>  
> +  s = new char(40);

Don't do this. It leaks memory. A much easier way to fix this is to simply declare s as a const char *.
(In reply to Jim Porter (:squib) from comment #2)
> I'd prefer setting rv to NS_OK when it's declared above.

OK.

> ::: mailnews/base/util/nsMsgKeySet.cpp
> > +  s = new char(40);
> 
> Don't do this. It leaks memory. A much easier way to fix this is to simply
> declare s as a const char *.

That doesn't work as the 's' pointer is changed and freed in the macros around.
I can try to free it before entering the macros.
Attached patch patch v1.1 (obsolete) — Splinter Review
Would this work?
Attachment #8697780 - Attachment is obsolete: true
Attachment #8697780 - Flags: review?(Pidgeot18)
Attachment #8697805 - Flags: feedback?(squibblyflabbetydoo)
No. I'm not sure why you can't use "const char *s;" but if that's impossible for some reason, you should use delete[] s;

Also, I just noticed that you're calling "s = new char(40);". That creates a single new character with the value 40 (i.e. the "(" character). You probably want "s = new char[40];" to create an array of 40 chars on the heap.
Attached patch patch v1.2 (obsolete) — Splinter Review
Thanks for the hints. I also changed strcat to strcpy. I ran the RunTests() code in the constructor of the msgKeySet and it did not abort, TB started fine and the console contained the output of all the tests. SO I assume the tests passed.
Attachment #8697805 - Attachment is obsolete: true
Attachment #8697805 - Flags: feedback?(squibblyflabbetydoo)
Attachment #8697809 - Flags: feedback?(squibblyflabbetydoo)
Comment on attachment 8697809 [details] [diff] [review]
patch v1.2

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

::: mailnews/base/util/nsMsgKeySet.cpp
@@ +1399,5 @@
>    nsMsgKeySet *set;
>    char *s;
>  
> +  s = new char[40];
> +  strcpy(s, "1-70,72-99,105,107,110-111,117-200");

Another way you could handle this, if you need s to be non-const, is to define s as:

  char s[] = "1-70,72-99,105,107,110-111,117-200";

Then, when you reassign s below, just declare a new variable instead.
Attachment #8697809 - Flags: feedback?(squibblyflabbetydoo) → feedback-
Attached patch patch v1.3 (obsolete) — Splinter Review
Better?
Attachment #8697809 - Attachment is obsolete: true
Attachment #8697860 - Flags: review?(Pidgeot18)
Attachment #8697860 - Flags: feedback?(squibblyflabbetydoo)
Comment on attachment 8697860 [details] [diff] [review]
patch v1.3

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

::: mailnews/base/util/nsMsgKeySet.cpp
@@ +74,5 @@
>    m_data = (int32_t *) PR_Malloc (sizeof (int32_t) * m_data_size);
>  #ifdef NEWSRC_DOES_HOST_STUFF
>    m_host = host;
>  #endif
> +  RunTests();

What is this change for? It's not in the previous patches, and seems to be out of the scope of fixing warnings.

@@ +1397,5 @@
>  void
>  nsMsgKeySet::test_member(bool with_cache)
>  {
>    nsMsgKeySet *set;
>    char *s;

You don't need this declaration anymore, do you?

@@ +1399,5 @@
>  {
>    nsMsgKeySet *set;
>    char *s;
>  
> +  const char *st1 = "1-70,72-99,105,107,110-111,117-200";

I'd lean towards:

  const char st1[] = "...";

but I doubt it matters much.
Attachment #8697860 - Flags: feedback?(squibblyflabbetydoo) → feedback+
(In reply to Jim Porter (:squib) from comment #9)
> ::: mailnews/base/util/nsMsgKeySet.cpp
> @@ +74,5 @@
> >    m_data = (int32_t *) PR_Malloc (sizeof (int32_t) * m_data_size);
> >  #ifdef NEWSRC_DOES_HOST_STUFF
> >    m_host = host;
> >  #endif
> > +  RunTests();
> 
> What is this change for? It's not in the previous patches, and seems to be
> out of the scope of fixing warnings.

It is proof the the tests pass with my other changes :)
Yes, this will not be in the final version.

> 
> @@ +1397,5 @@
> >  void
> >  nsMsgKeySet::test_member(bool with_cache)
> >  {
> >    nsMsgKeySet *set;
> >    char *s;
> 
> You don't need this declaration anymore, do you?

's' is used in the "TEST" macro, but it is moz_xmaloc'ed (in Output method) and free'd (in the macro) so I think we don't need to manage it in test_member.

> I'd lean towards:
> 
>   const char st1[] = "...";
> 
> but I doubt it matters much.

I think we use char * throughout the codebase.
I've added a patch in bug 1232306 that also fixes the compiler warnings "ISO C++11 does not allow conversion from string literal to 'char *' [-Wwritable-strings]" in an ISO-C++-compliant way without memory leaks etc. :-)
Comment on attachment 8697860 [details] [diff] [review]
patch v1.3

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

::: mailnews/base/util/nsMsgKeySet.cpp
@@ +74,5 @@
>    m_data = (int32_t *) PR_Malloc (sizeof (int32_t) * m_data_size);
>  #ifdef NEWSRC_DOES_HOST_STUFF
>    m_host = host;
>  #endif
> +  RunTests();

I'm guessing you added this because the tests weren't being run. But this change makes it so that we always run tests whenever someone uses this constructor in production code, which is at least once per newsgroup and possibly once per newsgroup per header download.

::: mailnews/compose/src/nsMsgSendLater.cpp
@@ +1012,5 @@
>          int32_t headLen = PL_strlen(HEADER_X_MOZILLA_STATUS2);
>          if (headLen == end - buf &&
>            !PL_strncasecmp(HEADER_X_MOZILLA_STATUS2, buf, end - buf))
>            prune_p = true;
> +        else if ((int32_t)PL_strlen(HEADER_X_MOZILLA_STATUS) == end - buf &&

There's really no reason to be using PL_strlen here, it should be strlen.

::: mailnews/imap/src/nsIMAPBodyShell.cpp
@@ +1308,5 @@
>  
>  nsIMAPMessagePartID::nsIMAPMessagePartID(nsIMAPeFetchFields fields, const char *partNumberString)
>  {
> +  m_fields = fields;
> +  m_partNumberString = partNumberString;

If you're bothering to change these lines, you should use the C++ initializer notation for fields instead, i.e.:

: m_fields(fields),
  m_partNumberString(partNumberString)
Attached patch patch v1.4 (obsolete) — Splinter Review
Adding the RunTests was not intended to be preserved in the patch. I only run them locally to test validity of the changed.
Attachment #8697860 - Attachment is obsolete: true
Attachment #8697860 - Flags: review?(Pidgeot18)
Attachment #8698658 - Flags: review?(Pidgeot18)
Depends on: 1238605
Depends on: 1238615, 1238608
Attached patch patch v1.5Splinter Review
Removed some of the blocks that are already reviewed in other bugs.
Attachment #8698658 - Attachment is obsolete: true
Attachment #8698658 - Flags: review?(Pidgeot18)
Attachment #8708760 - Flags: review?(Pidgeot18)
Comment on attachment 8708760 [details] [diff] [review]
patch v1.5

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

::: mailnews/imap/src/nsIMAPBodyShell.cpp
@@ +1326,5 @@
> +  uint32_t n = Length();
> +  for (uint32_t i = 0; i < n; i++)
> +  {
> +     nsIMAPMessagePartID *part = GetPart(i);
> +     delete part;

Ugh. I know this isn't your fault, but I still need to vent.

Why are we using an array of heap allocations for things that are just a pointer/int combo? That's just inefficient. This class really should have been just an nsTArray<nsIMAPMessagePartID>, no pointers needed. :-(
Attachment #8708760 - Flags: review?(Pidgeot18) → review+
Blocks: 1241426
Thanks.
Keywords: checkin-needed
What's with the three dependencies of this bug? Is this really ready to land?
Keywords: checkin-needed
Yes, this can be landed (unless there is a merge conflict). Those dependencies should have standalone patches for parts split off from here.
Keywords: checkin-needed
So they aren't dependencies ;)
No longer depends on: 1238605, 1238608, 1238615
Status: ASSIGNED → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 46.0
Is this responsible for the new xpcshell imap failures? If yes, please fix or back out.
Flags: needinfo?(acelists)
Let's wait whether bug 1241238 fixes it.
Flags: needinfo?(acelists)
Yes, it seems the patch in the other bug solved it, so it was not caused by this patch.
https://treeherder.mozilla.org/#/jobs?repo=try-comm-central&revision=003d1666ce18
You need to log in before you can comment on or make changes to this bug.