Closed Bug 1006982 Opened 6 years ago Closed 6 years ago

editor/libeditor/html/nsHTMLEditor.cpp:657:18: error: variable 'rv' is used uninitialized whenever 'if' condition is false [-Wsometimes-uninitialized]

Categories

(Core :: DOM: Editor, defect)

All
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla32
Tracking Status
firefox31 --- unaffected
firefox32 --- fixed

People

(Reporter: cpeterson, Assigned: cpeterson)

References

(Blocks 1 open bug)

Details

(Keywords: regression)

Attachments

(2 files)

This -Wuninitialized warning is a regression from bug 1000959. nsresult rv is uninitialized when blockParent is not a TableElement or a ListItem:

In file included from obj-firefox/editor/libeditor/html/Unified_cpp_libeditor_html0.cpp:119:
editor/libeditor/html/nsHTMLEditor.cpp:657:18: error: variable 'rv' is used uninitialized whenever 'if' condition is false [-Werror,-Wsometimes-uninitialized]
      } else if (nsHTMLEditUtils::IsListItem(blockParent)) {
                 ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
editor/libeditor/html/nsHTMLEditor.cpp:663:25: note: uninitialized use occurs here
      NS_ENSURE_SUCCESS(rv, rv);
                        ^~
../../../dist/include/nsDebug.h:333:21: note: expanded from macro 'NS_ENSURE_SUCCESS'
    nsresult __rv = res; /* Don't evaluate |res| more than once */        \
                    ^
editor/libeditor/html/nsHTMLEditor.cpp:657:14: note: remove the 'if' if its condition is always true
      } else if (nsHTMLEditUtils::IsListItem(blockParent)) {
             ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
editor/libeditor/html/nsHTMLEditor.cpp:651:7: note: variable 'rv' is declared here
      nsresult rv;
      ^
Attachment #8418526 - Flags: review?(ehsan)
While I'm tinkering in nsHTMLEditor.cpp, change some ancient macros and mutable strings to proper functions.
Attachment #8418530 - Flags: review?(ehsan)
How did this pass try?  Don't we have -Werror on for editor/?
We do have FAIL_ON_WARNINGS turned on for editor/, but unfortunately we don't enable -Wsometimes-uninitialized.
Attachment #8418526 - Flags: review?(ehsan) → review+
Comment on attachment 8418530 [details] [diff] [review]
libeditor_refactor-anchor-utilities.patch

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

::: editor/libeditor/html/nsHTMLEditor.cpp
@@ +81,5 @@
>  using namespace mozilla::widget;
>  
> +// Some utilities to handle overloading of "A" tag for link and named anchor.
> +static bool
> +IsLinkTag(const nsString& s)

Did you mean nsAString&?  r=me either way.
Attachment #8418530 - Flags: review?(ehsan) → review+
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #4)
> > +static bool
> > +IsLinkTag(const nsString& s)
> 
> Did you mean nsAString&?  r=me either way.

I couldn't use nsAString& because it does not define an EqualsIgnoreCase() member function. I don't know why not.
https://hg.mozilla.org/mozilla-central/rev/f1ef4bceb593
https://hg.mozilla.org/mozilla-central/rev/b9dc81faaec5
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
(In reply to comment #5)
> (In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment
> #4)
> > > +static bool
> > > +IsLinkTag(const nsString& s)
> > 
> > Did you mean nsAString&?  r=me either way.
> 
> I couldn't use nsAString& because it does not define an EqualsIgnoreCase()
> member function. I don't know why not.

You're in our group of 7 billion minus a handful people.  ;-)
(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #3)
> We do have FAIL_ON_WARNINGS turned on for editor/, but unfortunately we
> don't enable -Wsometimes-uninitialized.

Should we, or does it have annoying false positives in practice?  If the compiler can't prove that it's always initialized, I'd think the code should be a bit more explicit about its assumptions (maybe changing if(X)/else if(Y) to if(X)/else{assert(Y)}).
Aryeh: I'm trying to enabled -Wuninitialized in bug 1001975.

Historically, gcc's -Wuninitialized has a well-deserved reputation for reporting false positives. The good news is that gcc 4.7 separated -Wuninitialized's less reliable checks into a new -Wmaybe-uninitialized warning. -Wuninitialized warnings are rarely false positives these days.

clang does not support -Wmaybe-uninitialized, but it has a clang-specific -Wsometimes-uninitialized warning. However, in clang's case, "sometimes" means "some code paths" and they are rarely false positives. gcc's -Wmaybe-uninitialized warning means "maybe, maybe not, I can't tell" and reports ~90% false positives.

B2G still uses gcc 4.4, which doesn't have -Wmaybe-uninitialized, so its -Wuninitialized warnings are mostly false positives.
You need to log in before you can comment on or make changes to this bug.