Closed Bug 820197 Opened 12 years ago Closed 12 years ago

Fix build warnings in accessible

Categories

(Core :: Disability Access APIs, defect)

x86_64
Linux
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla20

People

(Reporter: catalin.usurelu5, Assigned: catalin.usurelu5)

References

(Blocks 1 open bug)

Details

Attachments

(1 file, 5 obsolete files)

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
I'm a beginner developer and this is my first contribution to the Mozilla codebase.
Attachment #690627 - Attachment is obsolete: true
Attachment #690629 - Flags: review?(catalin.usurelu5)
Attachment #690629 - Flags: review?(catalin.usurelu5) → review?(surkov.alexander)
Attached file warnings (obsolete) —
Warnings
Blocks: cleana11y
Component: General → Disability Access APIs
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 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
Attachment #690629 - Flags: review?(surkov.alexander)
Assignee: nobody → catalin.usurelu5
Status: UNCONFIRMED → ASSIGNED
Ever confirmed: true
(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 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)
Attached patch Added requested changes (obsolete) — Splinter Review
Attachment #690878 - Flags: review?
Attachment #690878 - Flags: review? → review?(surkov.alexander)
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.
Attachment #690878 - Flags: review?(surkov.alexander) → review+
> @@ +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 on attachment 690878 [details] [diff] [review] Added requested changes >+ NS_ASSERTION(mEvents.Length() != 0, nit, you don't need the != 0
Attachment #690778 - Attachment is obsolete: true
Attachment #690778 - Flags: review?
Attachment #690629 - Attachment is obsolete: true
Attached patch Fixed patchSplinter Review
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.
Attachment #690633 - Attachment is obsolete: true
Attachment #690878 - Attachment is obsolete: true
Attachment #691288 - Flags: review?(surkov.alexander)
Comment on attachment 691288 [details] [diff] [review] Fixed patch Review of attachment 691288 [details] [diff] [review]: ----------------------------------------------------------------- r=me, thank you
Attachment #691288 - Flags: review?(surkov.alexander) → review+
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: