Closed Bug 1238608 Opened 4 years ago Closed 4 years ago

Compiler warning: possibly uninitialized |rv| - mailnews/base/src/nsMessenger.cpp

Categories

(Thunderbird :: Build Config, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 46.0

People

(Reporter: ishikawa, Assigned: ishikawa)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch warning-nsMessenger.patch (obsolete) — Splinter Review
During local testing of C-C TB, I noticed a compiler warning from nsMessenger.cpp.

As I looked, |rv| ought to be initialized during normal processing, but try telling it to the compiler and future community maintainers.

I simply set a default error value and added a comment.

This shuts up the compiler warning (e.g., gcc 5.3).
QA Contact: ishikawa
Attachment #8706424 - Flags: review?(mkmelin+mozilla)
Assignee: nobody → ishikawa
QA Contact: ishikawa
Comment on attachment 8706424 [details] [diff] [review]
warning-nsMessenger.patch

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

::: mailnews/base/src/nsMessenger.cpp
@@ +2026,4 @@
>    aValue.Truncate();
>  
>    if (!mStringBundle)
> +    rv = InitStringBundle();    // should set mStringBundle to non-nullptr if succeed

there's some extra spaces here, but i don't think the comment is really needed

@@ +2030,3 @@
>  
>    if (mStringBundle)
>      rv = mStringBundle->GetStringFromName(aStringName.get(), getter_Copies(aValue));

actually, looks like a correct warning. you could just add this here instead
else
  rv =  = NS_ERROR_FAILURE;
Comment on attachment 8706424 [details] [diff] [review]
warning-nsMessenger.patch

Clearing r?
Please provide and updated patch
Attachment #8706424 - Flags: review?(mkmelin+mozilla)
Blocks: 1232107
Here is the updated patch.

It compiles and tests fine under local 64-bit linux.

Building C-C TB on tryserver has had some mysterious infra issue for the last couple of weeks and so I can't really say there is no problem generally speaking on
OS X and Windows, but for this particular patch, I don't think
there are problems on OSX or Windows.

TIA
Attachment #8706424 - Attachment is obsolete: true
Attachment #8709006 - Flags: review?(mkmelin+mozilla)
Comment on attachment 8709006 [details] [diff] [review]
warning-nsMessenger.patch (take 2)

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

Drive-by review, haha. I'd solve the problem differently as indicated.

::: mailnews/base/src/nsMessenger.cpp
@@ +2030,5 @@
>  
>    if (mStringBundle)
>      rv = mStringBundle->GetStringFromName(aStringName.get(), getter_Copies(aValue));
> +  else
> +    rv =  NS_ERROR_FAILURE;

Nit: double space after =.
I'd initialise rv to NS_OK above. aValue is empty, so we're cool.
Comment on attachment 8709006 [details] [diff] [review]
warning-nsMessenger.patch (take 2)

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

Thx for the patch. r=mkmelin with just a single space, like Jorg pointed out
Attachment #8709006 - Flags: review?(mkmelin+mozilla) → review+
No longer blocks: 1232107
Here is the updated patch carryingg over r=mkmelin+mozilla

I will put checkin-needed keyword.

TIA
Attachment #8709006 - Attachment is obsolete: true
Attachment #8710696 - Flags: review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/bc02a7319f2d
Status: NEW → RESOLVED
Closed: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 46.0
You need to log in before you can comment on or make changes to this bug.