755048 - Fix build warnings under accessibility/

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Comment 1

[Josh Matthews [:jdm]]

Attachment: <required>

Comment 2

[Trevor Saunders (:tbsaunde)]

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 3

[alexander :surkov (:asurkov)]

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

Comment 4

[alexander :surkov (:asurkov)]

(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.

Comment 5

[Hubert Figuiere [:hub]]

(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).

Comment 6

[alexander :surkov (:asurkov)]

(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.

Comment 7

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

FWIW, elsewhere in the tree we use

while ((child = walker.NextChild()) && AppendChild(child)) {
  continue;
}

Comment 8

[alexander :surkov (:asurkov)]

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.

Comment 9

[Trevor Saunders (:tbsaunde)]

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 10

[Trevor Saunders (:tbsaunde)]

Comment on attachment 623821 [details] [diff] [review]
<required>

cancel review per previous comment.

Comment 12

[Hubert Figuiere [:hub]]

Attachment: Fix warnings for uninitiliazed variables and unhandled case. r=

Comment 13

[Hubert Figuiere [:hub]]

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

Comment 14

[David Bolter [:davidb] (NeedInfo me for attention)]

(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 15

[Trevor Saunders (:tbsaunde)]

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

Comment 16

[David Bolter [:davidb] (NeedInfo me for attention)]

As per IRC I like the idea of explicitly adding the remaining cases to the switch statements.

Comment 17

[Trevor Saunders (:tbsaunde)]

(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 18

[Hubert Figuiere [:hub]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/1deeb80069cf

Comment 19

[Hubert Figuiere [:hub]]

Comment on attachment 634093 [details] [diff] [review]
Fix warnings for uninitiliazed variables and unhandled case. r=

checked in a variant.

Comment 20

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/1deeb80069cf

Comment 21

[alexander :surkov (:asurkov)]

Hub, anything left?

Comment 22

[Nathan Froyd [:froydnj]]

I still see quite a number of warnings on x86-64/Linux, GCC 4.4.5 and 4.6.x.

Comment 23

[Hubert Figuiere [:hub]]

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]

Comment 24

[alexander :surkov (:asurkov)]

Do we want to fix all of them here? If so then it seems this bug won't be ever fixed.

Comment 25

[Josh Matthews [:jdm]]

I concur with surkov.