Last Comment Bug 820197 - Fix build warnings in accessible
: Fix build warnings in accessible
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: Disability Access APIs (show other bugs)
: Trunk
: x86_64 Linux
: -- normal (vote)
: mozilla20
Assigned To: Usurelu Catalin
:
Mentors:
Depends on:
Blocks: cleana11y 819664
  Show dependency treegraph
 
Reported: 2012-12-10 16:44 PST by Usurelu Catalin
Modified: 2012-12-18 07:55 PST (History)
4 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Fixed some warnings in /accessible/src/base/NotificationController.cpp (2.28 KB, text/plain)
2012-12-10 16:44 PST, Usurelu Catalin
no flags Details
Fixed some warnings in /accessible/src/base/NotificationController.cpp (2.28 KB, patch)
2012-12-10 16:51 PST, Usurelu Catalin
no flags Details | Diff | Splinter Review
warnings (701 bytes, text/plain)
2012-12-10 17:08 PST, Usurelu Catalin
no flags Details
Added assertion, removed cast and changed argument(because i removed the cast) (3.11 KB, patch)
2012-12-11 01:34 PST, Usurelu Catalin
no flags Details | Diff | Splinter Review
Added requested changes (3.99 KB, patch)
2012-12-11 07:53 PST, Usurelu Catalin
surkov.alexander: review+
Details | Diff | Splinter Review
Fixed patch (3.97 KB, patch)
2012-12-12 04:49 PST, Usurelu Catalin
surkov.alexander: review+
Details | Diff | Splinter Review

Description Usurelu Catalin 2012-12-10 16:44:16 PST
Created attachment 690627 [details]
Fixed some warnings in /accessible/src/base/NotificationController.cpp

User Agent: Mozilla/5.0 (X11; Linux x86_64) AppleWebKit/537.11 (KHTML, like Gecko) Chrome/23.0.1271.95 Safari/537.11

Steps to reproduce:

Fixed build warnings in /accessible/src/base/NotificationController.cpp
Comment 1 Usurelu Catalin 2012-12-10 16:51:15 PST
Created attachment 690629 [details] [diff] [review]
Fixed some warnings in /accessible/src/base/NotificationController.cpp

I'm a beginner developer and this is my first contribution to the Mozilla codebase.
Comment 2 Usurelu Catalin 2012-12-10 17:08:05 PST
Created attachment 690633 [details]
warnings

Warnings
Comment 3 Trevor Saunders (:tbsaunde) 2012-12-10 17:24:48 PST
Comment on attachment 690629 [details] [diff] [review]
Fixed some warnings in /accessible/src/base/NotificationController.cpp

> NotificationController::QueueEvent(AccEvent* aEvent)
> {
>-  NS_ASSERTION(aEvent->mAccessible && aEvent->mAccessible->IsApplication() ||
>+  NS_ASSERTION((aEvent->mAccessible && aEvent->mAccessible->IsApplication()) ||
>                aEvent->GetDocAccessible() == mDocument,

I half way feel like telling gcc to just shutup about this one, what do you think Alex?

>-      for (uint32_t jdx = aThisIndex - 1; jdx < aThisIndex; jdx--) {
>+      for (uint32_t jdx = aThisIndex - 1; jdx < (uint32_t)aThisIndex; jdx--) {

I'd prefer that you made the argument unsigned, and fixed up the loop where its called, rather than adding a cast.
Comment 4 alexander :surkov 2012-12-10 21:12:47 PST
Comment on attachment 690629 [details] [diff] [review]
Fixed some warnings in /accessible/src/base/NotificationController.cpp

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

canceling, I'd like to take a look at next round

::: accessible/src/base/NotificationController.cpp
@@ +317,5 @@
>  void
>  NotificationController::CoalesceEvents()
>  {
>    uint32_t numQueuedEvents = mEvents.Length();
> +  uint32_t tail = numQueuedEvents - 1;

I would add an assertion here that numQueuedEvents is not 0

@@ +521,5 @@
>  
>      // Do not emit any preceding selection events for same widget if they
>      // weren't coalesced yet.
>      if (aThisEvent->mEventType != nsIAccessibleEvent::EVENT_SELECTION_WITHIN) {
> +      for (uint32_t jdx = aThisIndex - 1; jdx < (uint32_t)aThisIndex; jdx--) {

please follow Trev's suggestion
Comment 5 alexander :surkov 2012-12-10 21:16:24 PST
(In reply to Trevor Saunders (:tbsaunde) from comment #3)

> >-  NS_ASSERTION(aEvent->mAccessible && aEvent->mAccessible->IsApplication() ||
> >+  NS_ASSERTION((aEvent->mAccessible && aEvent->mAccessible->IsApplication()) ||
> >                aEvent->GetDocAccessible() == mDocument,
> 
> I half way feel like telling gcc to just shutup about this one, what do you
> think Alex?

extra parentheses are not necessary but it should be ok. At least we were warned that assignee and reviewer could miss a wrong logic.
Comment 6 Usurelu Catalin 2012-12-11 01:34:02 PST
Created attachment 690778 [details] [diff] [review]
Added assertion, removed cast and changed argument(because i removed the cast)
Comment 7 alexander :surkov 2012-12-11 03:34:51 PST
Comment on attachment 690778 [details] [diff] [review]
Added assertion, removed cast and changed argument(because i removed the cast)

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

::: accessible/src/base/NotificationController.cpp
@@ +319,5 @@
>  {
>    uint32_t numQueuedEvents = mEvents.Length();
> +  NS_ASSERTION(numQueuedEvents != 0,
> +               "There should be at least one pending event");
> +  uint32_t tail = numQueuedEvents - 1;

sorry for saying this late but you don't need numQueuedEvents variable at all

@@ +509,5 @@
>  
>  void
>  NotificationController::CoalesceSelChangeEvents(AccSelChangeEvent* aTailEvent,
>                                                  AccSelChangeEvent* aThisEvent,
> +                                                uint32_t aThisIndex)

pls fix a caller as well (int32_t 'for' loop)
Comment 8 Usurelu Catalin 2012-12-11 07:53:40 PST
Created attachment 690878 [details] [diff] [review]
Added requested changes
Comment 9 alexander :surkov 2012-12-11 18:46:58 PST
Comment on attachment 690878 [details] [diff] [review]
Added requested changes

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

r=me with comment fixed

::: accessible/src/base/NotificationController.cpp
@@ +318,5 @@
>  NotificationController::CoalesceEvents()
>  {
> +  NS_ASSERTION(mEvents.Length() != 0,
> +               "There should be at least one pending event");
> +  uint32_t tail = mEvents.Length()  - 1;

extra whitespace before '-'

@@ +394,5 @@
>  
>      case AccEvent::eCoalesceSelectionChange:
>      {
>        AccSelChangeEvent* tailSelChangeEvent = downcast_accEvent(tailEvent);
> +      for (uint32_t index = tail - 1; index >= 0; index--) {

index < tail, the idea here, when index is 0 then (index--) is 0xfff... which is always greater than mEvents.Length() - 1.
Comment 10 Trevor Saunders (:tbsaunde) 2012-12-11 18:50:16 PST
> @@ +394,5 @@
> >  
> >      case AccEvent::eCoalesceSelectionChange:
> >      {
> >        AccSelChangeEvent* tailSelChangeEvent = downcast_accEvent(tailEvent);
> > +      for (uint32_t index = tail - 1; index >= 0; index--) {
> 
> index < tail, the idea here, when index is 0 then (index--) is 0xfff...
> which is always greater than mEvents.Length() - 1.



but more importantly there is no such thing as a unsigned value < 0.
Comment 11 Trevor Saunders (:tbsaunde) 2012-12-11 18:51:11 PST
Comment on attachment 690878 [details] [diff] [review]
Added requested changes

>+  NS_ASSERTION(mEvents.Length() != 0,

nit, you don't need the != 0
Comment 12 Usurelu Catalin 2012-12-12 04:49:36 PST
Created attachment 691288 [details] [diff] [review]
Fixed patch

I don't know how I missed the unsigned > 0 part. The code was already written that way and I didn't think something so easy could be bugged.
Comment 13 alexander :surkov 2012-12-12 06:00:01 PST
Comment on attachment 691288 [details] [diff] [review]
Fixed patch

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

r=me, thank you
Comment 14 Ryan VanderMeulen [:RyanVM] 2012-12-16 16:52:26 PST
https://hg.mozilla.org/integration/mozilla-inbound/rev/9637f34d0df0
Comment 15 Ed Morley [:emorley] 2012-12-17 05:54:06 PST
https://hg.mozilla.org/mozilla-central/rev/9637f34d0df0

Note You need to log in before you can comment on or make changes to this bug.