819280 - Fix GCC warnings in Windows accessibility code

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[Jacek Caban]

Attachment: fix

Now that accessibility compiles with mingw, we may fix some warnings. Most of them are noise (like -Wreorder, -Wsign-compare), but there are some bits worth clean up (unused variable, missing MOZ_FINAL) and one real problem (deleting interface pointer instead of object instance pointer).

Comment 1

[Trevor Saunders (:tbsaunde)]

Comment on attachment 689637 [details] [diff] [review]
fix

>+  NS_ASSERTION(gWinEventMap[nsIAccessibleEvent::EVENT_LAST_ENTRY] == kEVENT_LAST_ENTRY,
>                "MSAA event map skewed");

it'd be nice if you converted this to a static assert while you were here.

>+  ChildrenEnumVariant(AccessibleWrap* aAnchor) : mRefCnt(0), mAnchorAcc(aAnchor),
>+    mCurAcc(mAnchorAcc->GetChildAt(0)), mCurIndex(0) { }

if we start care about win64 some day we should reorder the members for better packing instead of this, but it seems tricky to do with the macros, so don't worry about it.

>-  if (aTargetIndex < 0 || aTargetIndex >= mTargets.Length() || !aTarget)
>+  if (aTargetIndex < 0 || aTargetIndex >= (long)mTargets.Length() || !aTarget)

please cast aTargetIndex to unsigned since you just checked that its > 0.

>-  for (LONG i = 0; i < ArrayLength(ids); i++)
>+  for (LONG i = 0; i < (LONG)ArrayLength(ids); i++)

why not just use uint32_t ?

Comment 2

[Jacek Caban]

Thanks for review.

(In reply to Trevor Saunders (:tbsaunde) from comment #1)
> Comment on attachment 689637 [details] [diff] [review]
> fix
> 
> >+  NS_ASSERTION(gWinEventMap[nsIAccessibleEvent::EVENT_LAST_ENTRY] == kEVENT_LAST_ENTRY,
> >                "MSAA event map skewed");
> 
> it'd be nice if you converted this to a static assert while you were here.

This can't be simply replaced to static assert, because accessing an element of gWinEventMap is not evaluated statically. We could have instead a static assert using sizeof(gWinEventMap) and comparing its elem count to nsIAccessibleEvent::EVENT_LAST_ENTRY, but this would test a different thing.

> >-  if (aTargetIndex < 0 || aTargetIndex >= mTargets.Length() || !aTarget)
> >+  if (aTargetIndex < 0 || aTargetIndex >= (long)mTargets.Length() || !aTarget)
> 
> please cast aTargetIndex to unsigned since you just checked that its > 0.

Fixed.

> >-  for (LONG i = 0; i < ArrayLength(ids); i++)
> >+  for (LONG i = 0; i < (LONG)ArrayLength(ids); i++)
> 
> why not just use uint32_t ?

Because &i is used as parameter to SafeArrayPutElement, which is of LONG* type.

Comment 3

[Jacek Caban]

https://hg.mozilla.org/integration/mozilla-inbound/rev/0ad2485491af

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to Jacek Caban from comment #2)
> Thanks for review.
> 
> (In reply to Trevor Saunders (:tbsaunde) from comment #1)
> > Comment on attachment 689637 [details] [diff] [review]
> > fix
> > 
> > >+  NS_ASSERTION(gWinEventMap[nsIAccessibleEvent::EVENT_LAST_ENTRY] == kEVENT_LAST_ENTRY,
> > >                "MSAA event map skewed");
> > 
> > it'd be nice if you converted this to a static assert while you were here.
> 
> This can't be simply replaced to static assert, because accessing an element
> of gWinEventMap is not evaluated statically. We could have instead a static
> assert using sizeof(gWinEventMap) and comparing its elem count to
> nsIAccessibleEvent::EVENT_LAST_ENTRY, but this would test a different thing.

true, but just as useful a thing imo, and then we could remove the last entry from the array.

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0ad2485491af

Comment 6

[Jacek Caban]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> true, but just as useful a thing imo, and then we could remove the last
> entry from the array.

OK, I will attach a patch (I hope it's fine to reopen this bug for such a small change).

Comment 7

[Jacek Caban]

Attachment: Change NS_ASSERTION to MOZ_STATIC_ASSERT

Comment 8

[Trevor Saunders (:tbsaunde)]

Comment on attachment 692259 [details] [diff] [review]
Change NS_ASSERTION to MOZ_STATIC_ASSERT

>+  MOZ_STATIC_ASSERT(sizeof(gWinEventMap)/sizeof(gWinEventMap[0]) == nsIAccessibleEvent::EVENT_LAST_ENTRY,
>+                    "MSAA event map skewed");

I guess you can't use ArrayLength() because of no constexpr?

thank you for fixing this, sory reviewing took a couple days.

Comment 9

[Jacek Caban]

(In reply to Trevor Saunders (:tbsaunde) from comment #8)
> I guess you can't use ArrayLength() because of no constexpr?

Yes, that was the reason.

https://hg.mozilla.org/integration/mozilla-inbound/rev/b62655d36be4

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/b62655d36be4