fix bunch of warning in accessibility

RESOLVED FIXED in mozilla32

Status

()

defect
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: surkov, Assigned: surkov)

Tracking

(Blocks 1 bug)

unspecified
mozilla32
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

Posted patch patch (obsolete) — Splinter Review
No description provided.
Attachment #8411286 - Flags: review?(jwei)
Comment on attachment 8411286 [details] [diff] [review]
patch

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

In general, it looks okay as long as the range of expected return values is within max(int32_t).  However, as with the last time I tried to fix the warnings locally, a bunch of tests failed.  Running the tests locally, at least bounds/test_zoom_text.html has some failing tests, and there may be others.

::: accessible/src/generic/HyperTextAccessible-inl.h
@@ +108,5 @@
>    }
>  }
>  
> +inline uint32_t
> +HyperTextAccessible::ConvertMagicOffset(int32_t aOffset) const

There's at least one usage of this (HyperTextAccessible::IsValidRange) that do range checks against < 0 which should probably be changed.  There's also instances of assignments of this result to int32_t variables (e.g. HyperTextAccessible::TextAttributes, line 815) that should be changed.
(In reply to Jonathan Wei [:jwei] from comment #1)
> In general, it looks okay as long as the range of expected return values is
> within max(int32_t).  However, as with the last time I tried to fix the
> warnings locally, a bunch of tests failed.  Running the tests locally, at
> least bounds/test_zoom_text.html has some failing tests, and there may be
> others.

right, I asked for review before try build was finished. I'll figure it out.
Posted patch patch2 (obsolete) — Splinter Review
ok, this one is green
Assignee: nobody → surkov.alexander
Attachment #8411286 - Attachment is obsolete: true
Attachment #8411286 - Flags: review?(me)
Attachment #8411975 - Flags: review?(me)
Comment on attachment 8411975 [details] [diff] [review]
patch2

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

::: accessible/src/generic/HyperTextAccessible-inl.h
@@ +107,5 @@
>      editor->Paste(nsIClipboard::kGlobalClipboard);
>    }
>  }
>  
> +inline uint32_t

The result is often set to an int32_t variable.  Would it be possible to fix all the call sites of ConvertMagicOffset?

@@ +114,5 @@
>    if (aOffset == nsIAccessibleText::TEXT_OFFSET_END_OF_TEXT)
>      return CharacterCount();
>  
>    if (aOffset == nsIAccessibleText::TEXT_OFFSET_CARET)
>      return CaretOffset();

CaretOffset() can return -1 if the caret offset doesn't exist.  Is it guaranteed that we won't hit this case in ConvertMagicOffset calls?

@@ +116,5 @@
>  
>    if (aOffset == nsIAccessibleText::TEXT_OFFSET_CARET)
>      return CaretOffset();
>  
> +  return aOffset < 0 ? std::numeric_limits<uint32_t>::max() : aOffset;

Oops, not sure how I missed the '0' edge case the first time around.

::: accessible/src/generic/HyperTextAccessible.cpp
@@ +420,5 @@
>      DOMPoint();
>  }
>  
>  int32_t
> +HyperTextAccessible::FindOffset(uint32_t aOffset, nsDirection aDirection,

There are instances where the result of FindOffset is used in another call to FindOffset as the argument aOffset (e.g. lines 536 and 552). If it's not guaranteed that this won't return a negative value in those cases, then it could result in undefined behaviour.

@@ +519,5 @@
>    return hyperTextOffset;
>  }
>  
>  int32_t
> +HyperTextAccessible::FindLineBoundary(uint32_t aOffset,

Similar to FindOffset, the result is used as the argument to another call (lines 656, 657 and 662, for example).
Posted patch patch3 (obsolete) — Splinter Review
few more warnings fixed
Attachment #8411975 - Attachment is obsolete: true
Attachment #8411975 - Flags: review?(jwei)
Attachment #8412132 - Flags: review?(jwei)
Comment on attachment 8412132 [details] [diff] [review]
patch3

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

Lines 665 (TextBeforeOffset) and 792 (TextAfterOffset) use FindLineBoundary with an int32_t argument without range checking.
Lines 631, 634, 637, 647 (TextBeforeOffset), 702, 711 (TextAtOffset), 768, 778, 781, 784 (TextAfterOffset) use FindWordBoundary with an int32_t argument without range checking.
I'm not sure if range checking is necessary in these cases, but it'd probably be a good idea to check them out.

::: accessible/src/generic/HyperTextAccessible-inl.h
@@ +107,5 @@
>    if (aOffset == nsIAccessibleText::TEXT_OFFSET_END_OF_TEXT)
>      return CharacterCount();
>  
>    if (aOffset == nsIAccessibleText::TEXT_OFFSET_CARET)
>      return CaretOffset();

It's still not clear to me whether we might hit a return value of -1 through regular usage of ConvertMagicOffset or not.

::: accessible/src/generic/HyperTextAccessible.cpp
@@ +586,5 @@
>          return aOffset;
>  
>        // Move to next line end (as down arrow and end key were pressed).
>        int32_t tmpOffset = FindOffset(aOffset, eDirNext, eSelectLine);
> +      if (tmpOffset< 0 || tmpOffset == static_cast<int32_t>(CharacterCount()))

Nit: spacing.
(In reply to Jonathan Wei [:jwei] from comment #6)
> Lines 665 (TextBeforeOffset) and 792 (TextAfterOffset) use FindLineBoundary
> with an int32_t argument without range checking.
> Lines 631, 634, 637, 647 (TextBeforeOffset), 702, 711 (TextAtOffset), 768,
> 778, 781, 784 (TextAfterOffset) use FindWordBoundary with an int32_t
> argument without range checking.
> I'm not sure if range checking is necessary in these cases, but it'd
> probably be a good idea to check them out.

it's one of rarest cases when I have so careful reviewer :) I think I have to fix FindOffset to avoid negative values.

> >    if (aOffset == nsIAccessibleText::TEXT_OFFSET_CARET)
> >      return CaretOffset();
> 
> It's still not clear to me whether we might hit a return value of -1 through
> regular usage of ConvertMagicOffset or not.

-1 should be casted to uint32 max value.
Summary: fix bunch of warning in hypertext accessible class → fix bunch of warning in accessibility
Posted patch patch4Splinter Review
Attachment #8412132 - Attachment is obsolete: true
Attachment #8412132 - Flags: review?(jwei)
Attachment #8412585 - Flags: review?(jwei)
Comment on attachment 8412585 [details] [diff] [review]
patch4

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

::: accessible/src/base/TextAttrs.cpp
@@ +49,5 @@
>        Accessible* currAcc = mHyperTextAcc->GetChildAt(childIdx);
>        if (!nsAccUtils::IsEmbeddedObject(currAcc))
>          break;
>  
> +      (*aStartOffset)--;

We're likely not going to overflow on the (*aEndOffset)++, but is it possible to roll under 0 to uint32_t::max?

@@ +168,5 @@
>  
>      if (offsetFound)
>        break;
>  
> +    *(aStartOffset) -= nsAccUtils::TextLength(currAcc);

Same as above.

::: accessible/src/generic/HyperTextAccessible.cpp
@@ +232,5 @@
>    Accessible* endChild = GetChildAt(endChildIdx);
>    endChild->AppendTextTo(aText, 0, endOffset - endChildOffset);
>  }
>  
> +uint32_t

The return value is often set to int32_t variables, but it should be okay since our range is within [0, int32_t::max].

@@ +596,4 @@
>      }
>    }
>  
>    return -1;

This should probably be changed to 0 or whatever error value makes sense.
Attachment #8412585 - Flags: review?(jwei) → review+
(In reply to Jonathan Wei [:jwei] from comment #9)

> ::: accessible/src/base/TextAttrs.cpp
> > +      (*aStartOffset)--;
> 
> We're likely not going to overflow on the (*aEndOffset)++, but is it
> possible to roll under 0 to uint32_t::max?

it shouldn't be possible, it could happen if the tree was broken but in that case I wouldn't expect the correct behavior anyway.
    >diff --git a/accessible/src/base/AccGroupInfo.cpp b/accessible/src/base/AccGroupInfo.cpp
    >--- a/accessible/src/base/AccGroupInfo.cpp
    >+++ b/accessible/src/base/AccGroupInfo.cpp
    >@@ -193,17 +193,17 @@ AccGroupInfo::NextItemTo(Accessible* aIt
    >     return nullptr;
    > 
    >   // If the item in middle of the group then search next item in siblings.
    >   if (groupInfo->PosInSet() >= groupInfo->SetSize())
    >     return nullptr;
    > 
    >   Accessible* parent = aItem->Parent();
    >   uint32_t childCount = parent->ChildCount();
    >-  for (int32_t idx = aItem->IndexInParent() + 1; idx < childCount; idx++) {
    >+  for (uint32_t idx = aItem->IndexInParent() + 1; idx < childCount; idx++) {

    explain that, it seems like either IndexInParent() should return uint32_t or this should handle values less than 0 rather than silently acting as if it got something greater than 2^31 or invoking undefined behavior.

    >+++ b/accessible/src/base/nsAccessiblePivot.cpp
    >@@ -9,17 +9,17 @@
    > #include "HyperTextAccessible.h"
    > #include "nsAccUtils.h"
    > #include "States.h"
    > 
    > using namespace mozilla::a11y;
    > 
    > 
    > /**
    >- * An object that stores a given traversal rule during 
    >+ * An object that stores a given traversal rule during
    >  */
    > class RuleCache
    > {
    > public:
    >   RuleCache(nsIAccessibleTraversalRule* aRule) : mRule(aRule),
    >                                                  mAcceptRoles(nullptr) { }
    >   ~RuleCache () {
    >     if (mAcceptRoles)
    >@@ -320,17 +320,17 @@ nsAccessiblePivot::MoveNextByText(TextBo
    >     // starting on a text leaf), start the text movement from the end of that
    >     // node, otherwise we just default to 0.
    >     if (tempEnd == -1)
    >       tempEnd = text == curPosition->Parent() ?
    >                 text->GetChildOffset(curPosition) : 0;
    > 
    >     // If there's no more text on the current node, try to find the next text
    >     // node; if there isn't one, bail out.
    >-    if (tempEnd == text->CharacterCount()) {
    >+    if (tempEnd == static_cast<int32_t>(text->CharacterCount())) {

    why is tempEnd a int32_t? it seems like it should be some sort of option type where sum is unsigned.

    >@@ -384,17 +384,17 @@ nsAccessiblePivot::MoveNextByText(TextBo
    >         tempEnd = i;
    >         break;
    >       }
    >     }
    >     // If there's an embedded character at the very start of the range, we
    >     // instead want to traverse into it. So restart the movement with
    >     // the child as the starting point.
    >     if (childAtOffset && nsAccUtils::IsEmbeddedObject(childAtOffset) &&
    >-        tempStart == childAtOffset->StartOffset()) {
    >+        tempStart == static_cast<int32_t>(childAtOffset->StartOffset())) {

    same

    >     Accessible* startPosition = mPosition;
    >@@ -512,17 +512,17 @@ nsAccessiblePivot::MovePreviousByText(Te
    >         tempStart = childAtOffset->EndOffset();
    >         break;
    >       }
    >     }
    >     // If there's an embedded character at the very end of the range, we
    >     // instead want to traverse into it. So restart the movement with
    >     // the child as the starting point.
    >     if (childAtOffset && nsAccUtils::IsEmbeddedObject(childAtOffset) &&
    >-        tempEnd == childAtOffset->EndOffset()) {
    >+        tempEnd == static_cast<int32_t>(childAtOffset->EndOffset())) {
    here too, and probably more below.

    >+inline uint32_t
    >+HyperTextAccessible::ConvertMagicOffset(int32_t aOffset) const
    > {
    >   if (aOffset == nsIAccessibleText::TEXT_OFFSET_END_OF_TEXT)
    >     return CharacterCount();
    > 
    >   if (aOffset == nsIAccessibleText::TEXT_OFFSET_CARET)
    >     return CaretOffset();
    > 
    >-  return aOffset;
    >+  return aOffset < 0 ? std::numeric_limits<uint32_t>::max() : aOffset;

    I think I'd be happier if you wrapped the uint32_t in a struct that asserted people don't try to do math when the value is uint32_MAX, and maybe forced people to use .value() or something to get at the number.

    >@@ -274,17 +273,17 @@ HyperTextAccessible::DOMPointToOffset(ns
    >         if (aNode == GetNode()) {
    >           // Case #1: this accessible has no children and thus has empty text,
    >           // we can only be at hypertext offset 0.
    >           return 0;
    >         }
    > 
    >         // Case #2: there are no children, we're at this node.
    >         findNode = aNode;
    >-      } else if (aNodeOffset == aNode->GetChildCount()) {
    >+      } else if (static_cast<uint32_t>(aNodeOffset) == aNode->GetChildCount()) {

    ugh, I'm not sure casting is beter than leaving a warning here and then making aNodeOffset unsigned or something.

    >+uint32_t
    >+HyperTextAccessible::FindOffset(uint32_t aOffset, nsDirection aDirection,
    >                                 nsSelectionAmount aAmount,
    >                                 EWordMovementType aWordMovementType)
    > {
    >   // Find a leaf accessible frame to start with. PeekOffset wants this.
    >   HyperTextAccessible* text = this;
    >   Accessible* child = nullptr;
    >   int32_t innerOffset = aOffset;
    > 
    >   do {
    >     int32_t childIdx = text->GetChildIndexAtOffset(innerOffset);
    >     NS_ASSERTION(childIdx != -1, "Bad in offset!");
    >     if (childIdx == -1)
    >-      return -1;
    >+      return 0;

    just remove the code?

    >         *aEndOffset = FindWordBoundary(adjustedOffset, eDirPrevious, eStartWord);
    >         *aStartOffset = FindWordBoundary(*aEndOffset, eDirPrevious, eStartWord);
    >       } else {
    >         *aStartOffset = FindWordBoundary(adjustedOffset, eDirPrevious, eStartWord);
    >         *aEndOffset = FindWordBoundary(*aStartOffset, eDirNext, eStartWord);
    >-        if (*aEndOffset != adjustedOffset) {
    >+        if (*aEndOffset != static_cast<int32_t>(adjustedOffset)) {

    can we this function take uint32_t * instead, and have the platform wrappers cast if they need to?

    >@@ -807,49 +812,49 @@ HyperTextAccessible::TextAttributes(bool
    >   // 2. As we get each new attribute, we pass the current start and end offsets
    >   //    as in/out parameters. In other words, as attributes are collected,
    >   //    the attribute range itself can only stay the same or get smaller.
    > 
    >   *aStartOffset = *aEndOffset = 0;
    >   nsCOMPtr<nsIPersistentProperties> attributes =
    >     do_CreateInstance(NS_PERSISTENTPROPERTIES_CONTRACTID);
    > 
    >-  int32_t offset = ConvertMagicOffset(aOffset);
    >+  uint32_t offset = ConvertMagicOffset(aOffset);

    error check?

    >   int32_t accAtOffsetIdx = accAtOffset->IndexInParent();
    >-  int32_t startOffset = GetChildOffset(accAtOffsetIdx);
    >-  int32_t endOffset = GetChildOffset(accAtOffsetIdx + 1);
    >+  uint32_t startOffset = GetChildOffset(accAtOffsetIdx);
    >+  uint32_t endOffset = GetChildOffset(accAtOffsetIdx + 1);

    what? that function doesn't return uint32_t

    >+  NS_ASSERTION(startOffset < endOffset &&
    >+               endOffset != std::numeric_limits<uint32_t>::max(),
    >+               "Wrong bad in!");
    > 
    >   int32_t childIdx = GetChildIndexAtOffset(startOffset);
    >   if (childIdx == -1)
    >     return nsIntRect();
    > 
    >   nsIntRect bounds;
    >   int32_t prevOffset = GetChildOffset(childIdx);
    >   int32_t offset1 = startOffset - prevOffset;
    > 
    >-  while (childIdx < ChildCount()) {
    >+  while (childIdx < static_cast<int32_t>(ChildCount())) {

    why doesn't GetChildIndexAtOffset return unsigned option?

    > 
    >@@ -1356,17 +1363,17 @@ HyperTextAccessible::SelectionBoundsAt(i
    >                                        int32_t* aEndOffset)
    > {
    >   *aStartOffset = *aEndOffset = 0;
    > 
    >   nsTArray<nsRange*> ranges;
    >   GetSelectionDOMRanges(nsISelectionController::SELECTION_NORMAL, &ranges);
    > 
    >   uint32_t rangeCount = ranges.Length();
    >-  if (aSelectionNum < 0 || aSelectionNum >= rangeCount)
    >+  if (aSelectionNum < 0 || aSelectionNum >= static_cast<int32_t>(rangeCount))

    why don't we changed it to take uint32_t?

    >@@ -275,17 +275,17 @@ XULListboxAccessible::IsColSelected(uint
    >     do_QueryInterface(mContent);
    >   NS_ASSERTION(control,
    >                "Doesn't implement nsIDOMXULMultiSelectControlElement.");
    > 
    >   int32_t selectedrowCount = 0;
    >   nsresult rv = control->GetSelectedCount(&selectedrowCount);
    >   NS_ENSURE_SUCCESS(rv, false);
    > 
    >-  return selectedrowCount == RowCount();
    >+  return selectedrowCount == static_cast<int32_t>(RowCount());

    same as below

    >@@ -333,17 +333,18 @@ XULListboxAccessible::SelectedColCount()
    >     do_QueryInterface(mContent);
    >   NS_ASSERTION(control,
    >                "Doesn't implement nsIDOMXULMultiSelectControlElement.");
    > 
    >   int32_t selectedRowCount = 0;
    >   nsresult rv = control->GetSelectedCount(&selectedRowCount);
    >   NS_ENSURE_SUCCESS(rv, 0);
    > 
    >-  return selectedRowCount > 0 && selectedRowCount == RowCount() ? ColCount() : 0;
    >+  return selectedRowCount > 0 &&
    >+   selectedRowCount == static_cast<int32_t>(RowCount()) ? ColCount() : 0;

    I'd sort of rather leave the warning and wait for  GetSelectedRowCount() to return unisgned.
(In reply to Trevor Saunders (:tbsaunde) from comment #11)

>     >+  for (uint32_t idx = aItem->IndexInParent() + 1; idx < childCount;
> idx++) {
> 
>     explain that, it seems like either IndexInParent() should return
> uint32_t or this should handle values less than 0 rather than silently
> acting as if it got something greater than 2^31 or invoking undefined
> behavior.

-1 shouldn't happen here, if you want I can add an assertion here

>     >+    if (tempEnd == static_cast<int32_t>(text->CharacterCount())) {
> 
>     why is tempEnd a int32_t? it seems like it should be some sort of option
> type where sum is unsigned.

quite likely you're right but I do see they operate with negative values so I'm not touching this

>     >+  return aOffset < 0 ? std::numeric_limits<uint32_t>::max() : aOffset;
> 
>     I think I'd be happier if you wrapped the uint32_t in a struct that
> asserted people don't try to do math when the value is uint32_MAX, and maybe
> forced people to use .value() or something to get at the number.

I like the idea of assertion on attempt of MAX value usage but I don't like .value() stuff. Do you have something nice in your head?
Perhaps overload needed operators and assert if it has max value?

>     >+      } else if (static_cast<uint32_t>(aNodeOffset) ==
> aNode->GetChildCount()) {
> 
>     ugh, I'm not sure casting is beter than leaving a warning here and then
> making aNodeOffset unsigned or something.

I'll move casting to GetChildCount

>     >     int32_t childIdx = text->GetChildIndexAtOffset(innerOffset);
>     >     NS_ASSERTION(childIdx != -1, "Bad in offset!");
>     >     if (childIdx == -1)
>     >-      return -1;
>     >+      return 0;
> 
>     just remove the code?

do you mean keep assertion only? why?

>     >-        if (*aEndOffset != adjustedOffset) {
>     >+        if (*aEndOffset != static_cast<int32_t>(adjustedOffset)) {
> 
>     can we this function take uint32_t * instead, and have the platform
> wrappers cast if they need to?

yes, one more large change

>     >-  int32_t offset = ConvertMagicOffset(aOffset);
>     >+  uint32_t offset = ConvertMagicOffset(aOffset);
> 
>     error check?

right

>     >-  int32_t endOffset = GetChildOffset(accAtOffsetIdx + 1);
>     >+  uint32_t startOffset = GetChildOffset(accAtOffsetIdx);
>     >+  uint32_t endOffset = GetChildOffset(accAtOffsetIdx + 1);
> 
>     what? that function doesn't return uint32_t

it should not return a bad result here, but I agree it'd be nice to have some auto assertion

>     >-  return selectedRowCount > 0 && selectedRowCount == RowCount() ?
> ColCount() : 0;
>     >+  return selectedRowCount > 0 &&
>     >+   selectedRowCount == static_cast<int32_t>(RowCount()) ? ColCount() :
> 0;
> 
>     I'd sort of rather leave the warning and wait for  GetSelectedRowCount()
> to return unisgned.

I've got tired of warning
(In reply to alexander :surkov from comment #12)
> (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> 
> >     >+  for (uint32_t idx = aItem->IndexInParent() + 1; idx < childCount;
> > idx++) {
> > 
> >     explain that, it seems like either IndexInParent() should return
> > uint32_t or this should handle values less than 0 rather than silently
> > acting as if it got something greater than 2^31 or invoking undefined
> > behavior.
> 
> -1 shouldn't happen here, if you want I can add an assertion here

I'd rather you fixed IndexInParent so it returned uint32_t since really it should always be non negative, and if you really want to care about errors uint32option or something.

> >     >+    if (tempEnd == static_cast<int32_t>(text->CharacterCount())) {
> > 
> >     why is tempEnd a int32_t? it seems like it should be some sort of option
> > type where sum is unsigned.
> 
> quite likely you're right but I do see they operate with negative values so
> I'm not touching this

I really hope that's only -1, and if your casting something that can be negative that's just hiding a bug :(

> >     >+  return aOffset < 0 ? std::numeric_limits<uint32_t>::max() : aOffset;
> > 
> >     I think I'd be happier if you wrapped the uint32_t in a struct that
> > asserted people don't try to do math when the value is uint32_MAX, and maybe
> > forced people to use .value() or something to get at the number.
> 
> I like the idea of assertion on attempt of MAX value usage but I don't like
> .value() stuff. Do you have something nice in your head?

I like it being explicit, but I guess we could use operators.

> Perhaps overload needed operators and assert if it has max value?

I think if you implement operator uint32_t() then you can assign it to a uint32_t implicitly but not a int32_t which seems pretty reasonable.

> >     >+      } else if (static_cast<uint32_t>(aNodeOffset) ==
> > aNode->GetChildCount()) {
> > 
> >     ugh, I'm not sure casting is beter than leaving a warning here and then
> > making aNodeOffset unsigned or something.
> 
> I'll move casting to GetChildCount

how does that help anything?

> >     >     int32_t childIdx = text->GetChildIndexAtOffset(innerOffset);
> >     >     NS_ASSERTION(childIdx != -1, "Bad in offset!");
> >     >     if (childIdx == -1)
> >     >-      return -1;
> >     >+      return 0;
> > 
> >     just remove the code?
> 
> do you mean keep assertion only? why?

have you ever heard of it tripping? if not its useless code right?

> >     >-        if (*aEndOffset != adjustedOffset) {
> >     >+        if (*aEndOffset != static_cast<int32_t>(adjustedOffset)) {
> > 
> >     can we this function take uint32_t * instead, and have the platform
> > wrappers cast if they need to?
> 
> yes, one more large change

I'd prefer we made as much stuff as we can actually correct.

> >     >-  int32_t offset = ConvertMagicOffset(aOffset);
> >     >+  uint32_t offset = ConvertMagicOffset(aOffset);
> > 
> >     error check?
> 
> right

that's why I want things to be explicit ;)

> >     >-  int32_t endOffset = GetChildOffset(accAtOffsetIdx + 1);
> >     >+  uint32_t startOffset = GetChildOffset(accAtOffsetIdx);
> >     >+  uint32_t endOffset = GetChildOffset(accAtOffsetIdx + 1);
> > 
> >     what? that function doesn't return uint32_t
> 
> it should not return a bad result here, but I agree it'd be nice to have
> some auto assertion

it seems like it shouldn't ever, other than maybe a way to fail.

> >     >-  return selectedRowCount > 0 && selectedRowCount == RowCount() ?
> > ColCount() : 0;
> >     >+  return selectedRowCount > 0 &&
> >     >+   selectedRowCount == static_cast<int32_t>(RowCount()) ? ColCount() :
> > 0;
> > 
> >     I'd sort of rather leave the warning and wait for  GetSelectedRowCount()
> > to return unisgned.
> 
> I've got tired of warning

if there isn't tons of them I'm willing to live with them.
(In reply to Trevor Saunders (:tbsaunde) from comment #13)

> I think if you implement operator uint32_t() then you can assign it to a
> uint32_t implicitly but not a int32_t which seems pretty reasonable.

right

> > >     >+      } else if (static_cast<uint32_t>(aNodeOffset) ==
> > > aNode->GetChildCount()) {
> > > 
> > >     ugh, I'm not sure casting is beter than leaving a warning here and then
> > > making aNodeOffset unsigned or something.
> > 
> > I'll move casting to GetChildCount
> 
> how does that help anything?

it doesn't but more readable I guess

> > >     just remove the code?
> > 
> > do you mean keep assertion only? why?
> 
> have you ever heard of it tripping? if not its useless code right?

details pls?

> > >     >-        if (*aEndOffset != adjustedOffset) {
> > >     >+        if (*aEndOffset != static_cast<int32_t>(adjustedOffset)) {
> > > 
> > >     can we this function take uint32_t * instead, and have the platform
> > > wrappers cast if they need to?
> > 
> > yes, one more large change
> 
> I'd prefer we made as much stuff as we can actually correct.

I will file a follow up on this
https://hg.mozilla.org/mozilla-central/rev/527c58711da1
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla32
You need to log in before you can comment on or make changes to this bug.