Closed Bug 755048 Opened 7 years ago Closed 7 years ago

Fix build warnings under accessibility/

Categories

(Core :: Disability Access APIs, defect)

defect
Not set

Tracking

()

RESOLVED FIXED

People

(Reporter: jdm, Assigned: jdm)

References

(Blocks 1 open bug)

Details

Attachments

(2 files)

No description provided.
Attached patch <required>Splinter Review
Attachment #623821 - Flags: review?(surkov.alexander)
Comment on attachment 623821 [details] [diff] [review]
<required>

>-  MOZ_STATIC_ASSERT(roles::geckoRole == nsIAccessibleRole::ROLE_ ## geckoRole, "internal and xpcom roles differ!");
>+  MOZ_STATIC_ASSERT(roles::geckoRole == static_cast<roles::Role>(nsIAccessibleRole::ROLE_ ## geckoRole), "internal and xpcom roles differ!");

in practice I doubt that breaks anything, but it doesn't really seem correct.  I don't really feel like checking the spec but I feel like this shouldn't warn because of enum promoting to integer types.

>   nsAccTreeWalker walker(doc, mContent, CanHaveAnonChildren());
> 
>   nsAccessible* child = nsnull;
>-  while ((child = walker.NextChild()) && AppendChild(child));
>+  while ((child = walker.NextChild()) && AppendChild(child)) ;

really? I'm not sure what  warning this actually fixes, but it seems like a warning we might want to tell the compiler to  STFU about.
Comment on attachment 623821 [details] [diff] [review]
<required>

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

redirecting request to Trevor since he has concerns

::: accessible/src/base/RoleAsserts.cpp
@@ +11,5 @@
>  
>  using namespace mozilla::a11y;
>  
>  #define ROLE(geckoRole, stringRole, atkRole, macRole, msaaRole, ia2Role) \
> +  MOZ_STATIC_ASSERT(roles::geckoRole == static_cast<roles::Role>(nsIAccessibleRole::ROLE_ ## geckoRole), "internal and xpcom roles differ!");

since you break 80 char per line restriction then keep second argument on new line please, here and below
Attachment #623821 - Flags: review?(surkov.alexander) → review?(trev.saunders)
(In reply to Trevor Saunders (:tbsaunde) from comment #2)

> >-  while ((child = walker.NextChild()) && AppendChild(child));
> >+  while ((child = walker.NextChild()) && AppendChild(child)) ;
> 
> really? I'm not sure what  warning this actually fixes, but it seems like a
> warning we might want to tell the compiler to  STFU about.

interesting, a warning because of whitespace.
Assignee: nobody → josh
(In reply to alexander :surkov from comment #4)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> 
> > >-  while ((child = walker.NextChild()) && AppendChild(child));
> > >+  while ((child = walker.NextChild()) && AppendChild(child)) ;
> > 
> > really? I'm not sure what  warning this actually fixes, but it seems like a
> > warning we might want to tell the compiler to  STFU about.
> 
> interesting, a warning because of whitespace.

Newer gcc tend to ask you to use curly braces like I have been advocating :-)

The warning is meant to avoid the mistake of putting a semi-colon in the right place. I'm actually surprised that the space is enough, I would expect at least for a new line to be needed  (and it would be clearer).
(In reply to Hub Figuiere [:hub] from comment #5)
> (In reply to alexander :surkov from comment #4)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > 
> > > >-  while ((child = walker.NextChild()) && AppendChild(child));
> > > >+  while ((child = walker.NextChild()) && AppendChild(child)) ;
> > > 
> > > really? I'm not sure what  warning this actually fixes, but it seems like a
> > > warning we might want to tell the compiler to  STFU about.
> > 
> > interesting, a warning because of whitespace.
> 
> Newer gcc tend to ask you to use curly braces like I have been advocating :-)

if we curly braces are assumed then it's better to use them.
FWIW, elsewhere in the tree we use

while ((child = walker.NextChild()) && AppendChild(child)) {
  continue;
}
wouldn't it be equivalent to
while ((child = walker.NextChild()) && AppendChild(child)) { }
If so then I think I would go with it rather than with that continue looks weird a little bit.
sorry about taking so long to get back to this.

I'd happily give you r=me on everything acept the cast in RoleAsserts.cpp and the while(); stuff.  However I don't have any good ideas for those for know, so I'm tempted to leave them, but if you like you can try and come up with something nice or get Alex to accept something :)
Comment on attachment 623821 [details] [diff] [review]
<required>

cancel review per previous comment.
Attachment #623821 - Flags: review?(trev.saunders)
Duplicate of this bug: 765449
Comment on attachment 634093 [details] [diff] [review]
Fix warnings for uninitiliazed variables and unhandled case. r=

See https://bugzilla.mozilla.org/show_bug.cgi?id=765449#c5
Attachment #634093 - Flags: review?(dbolter)
Attachment #634093 - Flags: review?(dbolter) → review+
(In reply to Trevor Saunders (:tbsaunde) from comment #9)
> sorry about taking so long to get back to this.
> 
> I'd happily give you r=me on everything acept the cast in RoleAsserts.cpp
> and the while(); stuff.  However I don't have any good ideas for those for
> know, so I'm tempted to leave them,

Do you mean leave them as changed in the patch, or leave them as is before the patch?

> but if you like you can try and come up
> with something nice or get Alex to accept something :)

Alexander what do you think?
Comment on attachment 634093 [details] [diff] [review]
Fix warnings for uninitiliazed variables and unhandled case. r=

>       coordVal = NSCoordSaturatingMultiply(percentageBase,
>                                            styleCoord.GetPercentValue());
>       break;
>     }
>+    default:
>+      break;

I'm not really thrilled with just ignoring other cases and assuming everything is fine, but at first glance it seems a couple other values might be reasonable (eStyleUnit_None, eStyleUnit_NULL, eStyleUnit_auto and eStyleUnit_int) which seems like enough adding a NS_NOTREACHED might be really anoying.  So I guess I'm sort of ok with this.

>     case eTextPosSuper:
>       nsAccUtils::SetAccAttr(aAttributes, nsGkAtoms::textPosition,
>                              NS_LITERAL_STRING("super"));
>       break;
>+
>+    default:
>+      break;

I really think you should just explicitly take care of eTextPosNone here instead.

>        nscoord coordValue = styleCoord.GetCoordValue();
>        return coordValue > 0 ?
>          eTextPosSuper :
>          (coordValue < 0 ? eTextPosSub : eTextPosBaseline);
>     }
>+
>+    default:
>+      break;

same thing as the other switchc based on this enum :/

>   PRUint32 childCount = ChildCount();
>   nscolor rowColor, prevRowColor;
>+  rowColor = 0;

nit, init it in the decl
As per IRC I like the idea of explicitly adding the remaining cases to the switch statements.
(In reply to David Bolter [:davidb] from comment #14)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> > sorry about taking so long to get back to this.
> > 
> > I'd happily give you r=me on everything acept the cast in RoleAsserts.cpp
> > and the while(); stuff.  However I don't have any good ideas for those for
> > know, so I'm tempted to leave them,
> 
> Do you mean leave them as changed in the patch, or leave them as is before
> the patch?

I meant leave them as is before the patch.
Comment on attachment 634093 [details] [diff] [review]
Fix warnings for uninitiliazed variables and unhandled case. r=

checked in a variant.
Attachment #634093 - Flags: checkin+
Depends on: 785226
Hub, anything left?
I still see quite a number of warnings on x86-64/Linux, GCC 4.4.5 and 4.6.x.
Yeah there is quite a number. Here is a sample of unique one I get on Linux.

nsMaiInterfaceText.cpp
In file included from /home/hub/source/mozilla/aurora-src/accessible/src/atk/AccessibleWrap.h:11:0,
                 from /home/hub/source/mozilla/aurora-src/accessible/src/atk/../generic/ApplicationAccessible.h:11,
                 from /home/hub/source/mozilla/aurora-src/accessible/src/atk/ApplicationAccessibleWrap.h:10,
                 from /home/hub/source/mozilla/aurora-src/accessible/src/atk/ApplicationAccessibleWrap.cpp:7:
/home/hub/source/mozilla/aurora-src/accessible/src/atk/../generic/Accessible.h:103:2397: warning: ‘virtual nsresult Accessible::GroupPosition(int32_t*, int32_t*, int32_t*)’ was hidden [-Woverloaded-virtual]
In file included from /home/hub/source/mozilla/aurora-src/accessible/src/atk/ApplicationAccessibleWrap.h:10:0,
                 from /home/hub/source/mozilla/aurora-src/accessible/src/atk/ApplicationAccessibleWrap.cpp:7:
/home/hub/source/mozilla/aurora-src/accessible/src/atk/../generic/ApplicationAccessible.h:69:20: warning:   by ‘virtual mozilla::a11y::GroupPos mozilla::a11y::ApplicationAccessible::GroupPosition()’ [-Woverloaded-virtual]

TextLeafAccessible.cpp
/home/hub/source/mozilla/aurora-src/accessible/src/generic/HyperTextAccessible.cpp: In member function ‘virtual nsresult HyperTextAccessible::GetSelectionBounds(int32_t, int32_t*, int32_t*)’:
/home/hub/source/mozilla/aurora-src/accessible/src/generic/HyperTextAccessible.cpp:1753:45: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
/home/hub/source/mozilla/aurora-src/accessible/src/generic/HyperTextAccessible.cpp: In member function ‘virtual nsresult HyperTextAccessible::SetSelectionBounds(int32_t, int32_t, int32_t)’:
/home/hub/source/mozilla/aurora-src/accessible/src/generic/HyperTextAccessible.cpp:1812:24: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
/home/hub/source/mozilla/aurora-src/accessible/src/generic/HyperTextAccessible.cpp:1822:24: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
In file included from /home/hub/source/mozilla/aurora-src/accessible/src/generic/../atk/AccessibleWrap.h:11:0,
                 from /home/hub/source/mozilla/aurora-src/accessible/src/generic/HyperTextAccessible.h:13,
                 from /home/hub/source/mozilla/aurora-src/accessible/src/generic/../base/nsCaretAccessible.h:9,
                 from /home/hub/source/mozilla/aurora-src/accessible/src/generic/RootAccessible.h:9,
                 from /home/hub/source/mozilla/aurora-src/accessible/src/generic/RootAccessible.cpp:6:
/home/hub/source/mozilla/aurora-src/accessible/src/generic/Accessible.h:103:2397: warning: ‘virtual nsresult Accessible::GroupPosition(int32_t*, int32_t*, int32_t*)’ was hidden [-Woverloaded-virtual]
In file included from /home/hub/source/mozilla/aurora-src/accessible/src/generic/RootAccessible.cpp:23:0:
/home/hub/source/mozilla/aurora-src/accessible/src/generic/../xul/XULTreeAccessible.h:160:20: warning:   by ‘virtual mozilla::a11y::GroupPos mozilla::a11y::XULTreeItemAccessibleBase::GroupPosition()’ [-Woverloaded-virtual]


HTMLTableAccessible.cpp
/home/hub/source/mozilla/aurora-src/accessible/src/html/HTMLTableAccessible.cpp: In member function ‘virtual uint32_t mozilla::a11y::HTMLTableAccessible::SelectedCellCount()’:
/home/hub/source/mozilla/aurora-src/accessible/src/html/HTMLTableAccessible.cpp:589:48: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
/home/hub/source/mozilla/aurora-src/accessible/src/html/HTMLTableAccessible.cpp:590:28: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
/home/hub/source/mozilla/aurora-src/accessible/src/html/HTMLTableAccessible.cpp: In member function ‘virtual void mozilla::a11y::HTMLTableAccessible::SelectedCells(nsTArray<Accessible*>*)’:
/home/hub/source/mozilla/aurora-src/accessible/src/html/HTMLTableAccessible.cpp:645:48: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
/home/hub/source/mozilla/aurora-src/accessible/src/html/HTMLTableAccessible.cpp:646:28: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
/home/hub/source/mozilla/aurora-src/accessible/src/html/HTMLTableAccessible.cpp: In member function ‘virtual void mozilla::a11y::HTMLTableAccessible::SelectedCellIndices(nsTArray<unsigned int, nsTArrayDefaultAllocator>*)’:
/home/hub/source/mozilla/aurora-src/accessible/src/html/HTMLTableAccessible.cpp:678:48: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
/home/hub/source/mozilla/aurora-src/accessible/src/html/HTMLTableAccessible.cpp:679:28: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
/home/hub/source/mozilla/aurora-src/accessible/src/html/HTMLTableAccessible.cpp: In member function ‘virtual void mozilla::a11y::HTMLTableAccessible::SelectRow(uint32_t)’:
/home/hub/source/mozilla/aurora-src/accessible/src/html/HTMLTableAccessible.cpp:860:12: warning: unused variable ‘rv’ [-Wunused-variable]
/home/hub/source/mozilla/aurora-src/accessible/src/html/HTMLTableAccessible.cpp: In member function ‘virtual void mozilla::a11y::HTMLTableAccessible::SelectCol(uint32_t)’:
/home/hub/source/mozilla/aurora-src/accessible/src/html/HTMLTableAccessible.cpp:873:12: warning: unused variable ‘rv’ [-Wunused-variable]

XULTreeGridAccessible.cpp
/home/hub/source/mozilla/aurora-src/accessible/src/xul/XULListboxAccessible.cpp: In member function ‘virtual bool mozilla::a11y::XULListboxAccessible::IsColSelected(uint32_t)’:
/home/hub/source/mozilla/aurora-src/accessible/src/xul/XULListboxAccessible.cpp:282:39: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
/home/hub/source/mozilla/aurora-src/accessible/src/xul/XULListboxAccessible.cpp: In member function ‘virtual uint32_t mozilla::a11y::XULListboxAccessible::SelectedColCount()’:
/home/hub/source/mozilla/aurora-src/accessible/src/xul/XULListboxAccessible.cpp:340:63: warning: comparison between signed and unsigned integer expressions [-Wsign-compare]
/home/hub/source/mozilla/aurora-src/accessible/src/xul/XULListboxAccessible.cpp: In member function ‘virtual void mozilla::a11y::XULListboxAccessible::SelectedCells(nsTArray<Accessible*>*)’:
/home/hub/source/mozilla/aurora-src/accessible/src/xul/XULListboxAccessible.cpp:372:12: warning: unused variable ‘rv’ [-Wunused-variable]
/home/hub/source/mozilla/aurora-src/accessible/src/xul/XULListboxAccessible.cpp: In member function ‘virtual void mozilla::a11y::XULListboxAccessible::SelectedCellIndices(nsTArray<unsigned int, nsTArrayDefaultAllocator>*)’:
/home/hub/source/mozilla/aurora-src/accessible/src/xul/XULListboxAccessible.cpp:406:12: warning: unused variable ‘rv’ [-Wunused-variable]
/home/hub/source/mozilla/aurora-src/accessible/src/xul/XULListboxAccessible.cpp: In member function ‘virtual void mozilla::a11y::XULListboxAccessible::SelectedRowIndices(nsTArray<unsigned int, nsTArrayDefaultAllocator>*)’:
/home/hub/source/mozilla/aurora-src/accessible/src/xul/XULListboxAccessible.cpp:455:12: warning: unused variable ‘rv’ [-Wunused-variable]
Do we want to fix all of them here? If so then it seems this bug won't be ever fixed.
I concur with surkov.
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Whiteboard: [leave open]
You need to log in before you can comment on or make changes to this bug.