1001623 - clean up error handling of index returning methods

Status: RESOLVED FIXED

(Core :: Disability Access APIs, defect)

Description

[alexander :surkov (:asurkov)]

spin off bug 1000465

Comment 1

[alexander :surkov (:asurkov)]

Attachment: patch

how does it look?

Comment 2

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8414598 [details] [diff] [review]
patch

>+/**
>+ * An index type. Assert if out of range value was attempted to be used.
>+ */
>+class index_t
>+{
>+public:
>+  index_t(int32_t aVal) : mVal(aVal) {}

I think it would be nice if the way to create an invalid index was clearer than Index_t(-1)

>+  operator uint32_t() const
>+  {
>+    NS_ASSERTION(mVal >= 0, "Attempt to use wrong index!");

I'd say it should probably be a fatal assert.

>+  /**
>+   * Return true if the value is within the range (0, aVal).
>+   */

that's not what <= means, so its confusing and worse lets you write stupidly slow code below.  If you want to compare an index to another number you should just use the operator uint32_t and assert its a a number first.

>+private:
>+  int32_t mVal;

why aren't you using size_t or ssize_t throughout?

> {
>-  uint32_t endOffset = ConvertMagicOffset(aEndOffset);
>-  return ConvertMagicOffset(aStartOffset) <= endOffset &&
>-    endOffset <= CharacterCount();
>+  index_t endOffset = ConvertMagicOffset(aEndOffset);
>+  return endOffset <= CharacterCount() &&
>+    ConvertMagicOffset(aStartOffset) <= endOffset;

example #1 of code that is slow for no good reason.  If end is valid then you don't need to compare it to ChildCount() and it being in the invalid state is the only way it can be > ChildCount()

Comment 3

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #2)

> I think it would be nice if the way to create an invalid index was clearer
> than Index_t(-1)

I though about WRONG_INDEX constant but having index_t(int32_t) is handy in bunch of cases, so I didn't introduce it

> I'd say it should probably be a fatal assert.

do you keep in mind macros for it?

> 
> >+  /**
> >+   * Return true if the value is within the range (0, aVal).
> >+   */
> 
> that's not what <= means,

if you take into account that index_t is supposed to be always greater than 0 then it's exactly what it means.

> so its confusing and worse lets you write stupidly
> slow code below.  If you want to compare an index to another number you
> should just use the operator uint32_t and assert its a a number first.

do you have an example? I don't follow you.

> >+private:
> >+  int32_t mVal;
> 
> why aren't you using size_t or ssize_t throughout?

what's for?

> example #1 of code that is slow for no good reason.  If end is valid then
> you don't need to compare it to ChildCount() and it being in the invalid
> state is the only way it can be > ChildCount()

sorry, I can't translate it, code example will be great.

Comment 4

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #3)
> (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> 
> > I think it would be nice if the way to create an invalid index was clearer
> > than Index_t(-1)
> 
> I though about WRONG_INDEX constant but having index_t(int32_t) is handy in
> bunch of cases, so I didn't introduce it

use a default ctor maybe? though I gues its kind of awkward if that means the index is 0 or invalid.

> > I'd say it should probably be a fatal assert.
> 
> do you keep in mind macros for it?

MOZ_ASSERT would be fine.

> > 
> > >+  /**
> > >+   * Return true if the value is within the range (0, aVal).
> > >+   */
> > 
> > that's not what <= means,
> 
> if you take into account that index_t is supposed to be always greater than
> 0 then it's exactly what it means.

accept its not because if it was you would have used a plain unsigned integer type.

> > so its confusing and worse lets you write stupidly
> > slow code below.  If you want to compare an index to another number you
> > should just use the operator uint32_t and assert its a a number first.
> 
> do you have an example? I don't follow you.

I gave one in my last comment...

> > >+private:
> > >+  int32_t mVal;
> > 
> > why aren't you using size_t or ssize_t throughout?
> 
> what's for?

because it makes more sense to use types that can actually represent any possible length?

> > example #1 of code that is slow for no good reason.  If end is valid then
> > you don't need to compare it to ChildCount() and it being in the invalid
> > state is the only way it can be > ChildCount()
> 
> sorry, I can't translate it, code example will be great.

I don't see what there is to not understand here. your introducing a call to ChildrenCount() in this hunk that pretty clearly serves no useful purpose.

Comment 5

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #4)
> (In reply to alexander :surkov from comment #3)
> > (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > 
> > > I think it would be nice if the way to create an invalid index was clearer
> > > than Index_t(-1)
> > 
> > I though about WRONG_INDEX constant but having index_t(int32_t) is handy in
> > bunch of cases, so I didn't introduce it
> 
> use a default ctor maybe? though I gues its kind of awkward if that means
> the index is 0 or invalid.
> 
> > > I'd say it should probably be a fatal assert.
> > 
> > do you keep in mind macros for it?
> 
> MOZ_ASSERT would be fine.
> 
> > > 
> > > >+  /**
> > > >+   * Return true if the value is within the range (0, aVal).
> > > >+   */
> > > 
> > > that's not what <= means,
> > 
> > if you take into account that index_t is supposed to be always greater than
> > 0 then it's exactly what it means.
> 
> accept its not because if it was you would have used a plain unsigned
> integer type.

negative value means invalid value which is incoparable with valid value in general, so if <= returns false when you try to compare incoparable things then it's reasonable as long as you don't make a conclusion than invalid > valid. I wouldn't like to go with exception which sounds also reasonable for comparing the incomparables. Aleternatively I could go with method like isInRange but not sure if it's nicer.

> > > why aren't you using size_t or ssize_t throughout?
> > 
> > what's for?
> 
> because it makes more sense to use types that can actually represent any
> possible length?

what about invalid values?

> > > example #1 of code that is slow for no good reason.  If end is valid then
> > > you don't need to compare it to ChildCount() and it being in the invalid
> > > state is the only way it can be > ChildCount()
> > 
> > sorry, I can't translate it, code example will be great.
> 
> I don't see what there is to not understand here. your introducing a call to
> ChildrenCount() in this hunk that pretty clearly serves no useful purpose.

it was language issue. I didn't introduce ChildrenCount check. I don't follow why it's useless.

Comment 6

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #5)
> (In reply to Trevor Saunders (:tbsaunde) from comment #4)
> > (In reply to alexander :surkov from comment #3)
> > > (In reply to Trevor Saunders (:tbsaunde) from comment #2)
> > > 
> > > > I think it would be nice if the way to create an invalid index was clearer
> > > > than Index_t(-1)
> > > 
> > > I though about WRONG_INDEX constant but having index_t(int32_t) is handy in
> > > bunch of cases, so I didn't introduce it
> > 
> > use a default ctor maybe? though I gues its kind of awkward if that means
> > the index is 0 or invalid.
> > 
> > > > I'd say it should probably be a fatal assert.
> > > 
> > > do you keep in mind macros for it?
> > 
> > MOZ_ASSERT would be fine.
> > 
> > > > 
> > > > >+  /**
> > > > >+   * Return true if the value is within the range (0, aVal).
> > > > >+   */
> > > > 
> > > > that's not what <= means,
> > > 
> > > if you take into account that index_t is supposed to be always greater than
> > > 0 then it's exactly what it means.
> > 
> > accept its not because if it was you would have used a plain unsigned
> > integer type.
> 
> negative value means invalid value which is incoparable with valid value in
> general, so if <= returns false when you try to compare incoparable things
> then it's reasonable as long as you don't make a conclusion than invalid >

I'd say since comparing incomparable things doesn't make any sense there's no reasonable thing for this operator to return.

> valid. I wouldn't like to go with exception which sounds also reasonable for

I don't understand this last bit.

> comparing the incomparables. Aleternatively I could go with method like
> isInRange but not sure if it's nicer.

just call it Valid()? I'd say its a lot clearer.
> 
> > > > why aren't you using size_t or ssize_t throughout?
> > > 
> > > what's for?
> > 
> > because it makes more sense to use types that can actually represent any
> > possible length?
> 
> what about invalid values?

just treat SIZE_MAX or whatever the name is specially, you know you can't have a string that long anyway because if you di then there wouldn't be space for the length you're storing.

> > > > example #1 of code that is slow for no good reason.  If end is valid then
> > > > you don't need to compare it to ChildCount() and it being in the invalid
> > > > state is the only way it can be > ChildCount()
> > > 
> > > sorry, I can't translate it, code example will be great.
> > 
> > I don't see what there is to not understand here. your introducing a call to
> > ChildrenCount() in this hunk that pretty clearly serves no useful purpose.
> 
> it was language issue. I didn't introduce ChildrenCount check. I don't
> follow why it's useless.

ConvertMagicOffset() returns [ 0, ChildrenCount() ] || none right? therefore if end is not none then clearly end is less than or equal to  ChildrenCount().

Comment 7

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #6)

> > negative value means invalid value which is incoparable with valid value in
> > general, so if <= returns false when you try to compare incoparable things
> > then it's reasonable as long as you don't make a conclusion than invalid >
> 
> I'd say since comparing incomparable things doesn't make any sense there's
> no reasonable thing for this operator to return.
> 
> > valid. I wouldn't like to go with exception which sounds also reasonable for
> 
> I don't understand this last bit.

I meant to throw an exception

> > comparing the incomparables. Aleternatively I could go with method like
> > isInRange but not sure if it's nicer.
> 
> just call it Valid()? I'd say its a lot clearer.

I still need to check boundaries. I can go with isInRange you don't super like operators approach.

> just treat SIZE_MAX or whatever the name is specially, you know you can't
> have a string that long anyway because if you di then there wouldn't be
> space for the length you're storing.

I see, but there's no real benefits, right, anyway I introduce own type, on the another hand APIs deal with int32_t and I'd need to convert negative int32_t to SIZE_MAX

> > > > > example #1 of code that is slow for no good reason.  If end is valid then
> > > > > you don't need to compare it to ChildCount() and it being in the invalid
> > > > > state is the only way it can be > ChildCount()
> > > > 
> > > > sorry, I can't translate it, code example will be great.
> > > 
> > > I don't see what there is to not understand here. your introducing a call to
> > > ChildrenCount() in this hunk that pretty clearly serves no useful purpose.
> > 
> > it was language issue. I didn't introduce ChildrenCount check. I don't
> > follow why it's useless.
> 
> ConvertMagicOffset() returns [ 0, ChildrenCount() ] || none right?

ConverMagicOffset may return any value from (-infinity, +infinity)

Comment 8

[alexander :surkov (:asurkov)]

Attachment: patch2

Comment 9

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #7)
> (In reply to Trevor Saunders (:tbsaunde) from comment #6)
> 
> > > negative value means invalid value which is incoparable with valid value in
> > > general, so if <= returns false when you try to compare incoparable things
> > > then it's reasonable as long as you don't make a conclusion than invalid >
> > 
> > I'd say since comparing incomparable things doesn't make any sense there's
> > no reasonable thing for this operator to return.
> > 
> > > valid. I wouldn't like to go with exception which sounds also reasonable for
> > 
> > I don't understand this last bit.
> 
> I meant to throw an exception

I don't see why a type system should allow compareing in  comperable things when it isn't necessary.

> > > comparing the incomparables. Aleternatively I could go with method like
> > > isInRange but not sure if it's nicer.
> > 
> > just call it Valid()? I'd say its a lot clearer.
> 
> I still need to check boundaries. I can go with isInRange you don't super
> like operators approach.

I mean after you know its valid you can convert to an unsigned type and use the built in operators.

> > just treat SIZE_MAX or whatever the name is specially, you know you can't
> > have a string that long anyway because if you di then there wouldn't be
> > space for the length you're storing.
> 
> I see, but there's no real benefits, right, anyway I introduce own type, on

well, all this stuff should ideally operate on size_t, so just slightly more sanity and better for future.

> the another hand APIs deal with int32_t and I'd need to convert negative
> int32_t to SIZE_MAX

the only negative value you get is -1 (though you should assert that) so just cast it to size_t and you get SIZE_MAX (though you need to make sure its a type of cast that will do sign extension I think we rely on function style casts doing that in nsTArray already)

> > > > > > example #1 of code that is slow for no good reason.  If end is valid then
> > > > > > you don't need to compare it to ChildCount() and it being in the invalid
> > > > > > state is the only way it can be > ChildCount()
> > > > > 
> > > > > sorry, I can't translate it, code example will be great.
> > > > 
> > > > I don't see what there is to not understand here. your introducing a call to
> > > > ChildrenCount() in this hunk that pretty clearly serves no useful purpose.
> > > 
> > > it was language issue. I didn't introduce ChildrenCount check. I don't
> > > follow why it's useless.
> > 
> > ConvertMagicOffset() returns [ 0, ChildrenCount() ] || none right?
> 
> ConverMagicOffset may return any value from (-infinity, +infinity)

that's totally not true.  if you look at the return type its { -2^31, 2^31 - 1}.  THe only values that make sense are error or {0, CharacterCount() }.  It would be nice if ConvertMagicOffset() could check the passed offset was less than CharacterCount() but I guess we might want to keep it the way it is because that can be faster in some cases.

Comment 10

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #9)

> > I still need to check boundaries. I can go with isInRange you don't super
> > like operators approach.
> 
> I mean after you know its valid you can convert to an unsigned type and use
> the built in operators.

yeah or have bunch of shortcut methods as the last patch does

> > the another hand APIs deal with int32_t and I'd need to convert negative
> > int32_t to SIZE_MAX
> 
> the only negative value you get is -1 (though you should assert that)

per ConvertMagicOffset impl it can take any negative value, no?

Comment 11

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #10)
> (In reply to Trevor Saunders (:tbsaunde) from comment #9)
> 
> > > I still need to check boundaries. I can go with isInRange you don't super
> > > like operators approach.
> > 
> > I mean after you know its valid you can convert to an unsigned type and use
> > the built in operators.
> 
> yeah or have bunch of shortcut methods as the last patch does

I'd say its easier to think about the case the value is error separately, but I suppose that's true.

> > > the another hand APIs deal with int32_t and I'd need to convert negative
> > > int32_t to SIZE_MAX
> > 
> > the only negative value you get is -1 (though you should assert that)
> 
> per ConvertMagicOffset impl it can take any negative value, no?

we might as well canonicalize though, and really there's more places that a unsigned or error type would be useful than the return of ConvertMagicOffset() and in some of those we control everything for example IndexInParent.

Comment 12

[alexander :surkov (:asurkov)]

(In reply to Trevor Saunders (:tbsaunde) from comment #11)

> > > I mean after you know its valid you can convert to an unsigned type and use
> > > the built in operators.
> > 
> > yeah or have bunch of shortcut methods as the last patch does
> 
> I'd say its easier to think about the case the value is error separately,
> but I suppose that's true.

IsValid is flexible approach, shortcut methods are shorter. I don't have a preference I think.

> > > > the another hand APIs deal with int32_t and I'd need to convert negative
> > > > int32_t to SIZE_MAX
> > > 
> > > the only negative value you get is -1 (though you should assert that)
> > 
> > per ConvertMagicOffset impl it can take any negative value, no?
> 
> we might as well canonicalize though, and really there's more places that a
> unsigned or error type would be useful than the return of
> ConvertMagicOffset() and in some of those we control everything for example
> IndexInParent.

right, or use int32_t for index_t since it works both for ConverMagicOffset and IndexInParent

Comment 13

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #12)
> (In reply to Trevor Saunders (:tbsaunde) from comment #11)
> 
> > > > I mean after you know its valid you can convert to an unsigned type and use
> > > > the built in operators.
> > > 
> > > yeah or have bunch of shortcut methods as the last patch does
> > 
> > I'd say its easier to think about the case the value is error separately,
> > but I suppose that's true.
> 
> IsValid is flexible approach, shortcut methods are shorter. I don't have a
> preference I think.

I'd go for something like Isvalid.

> 
> > > > > the another hand APIs deal with int32_t and I'd need to convert negative
> > > > > int32_t to SIZE_MAX
> > > > 
> > > > the only negative value you get is -1 (though you should assert that)
> > > 
> > > per ConvertMagicOffset impl it can take any negative value, no?
> > 
> > we might as well canonicalize though, and really there's more places that a
> > unsigned or error type would be useful than the return of
> > ConvertMagicOffset() and in some of those we control everything for example
> > IndexInParent.
> 
> right, or use int32_t for index_t since it works both for ConverMagicOffset
> and IndexInParent

size_t internal representation should work for both too imo.

Comment 14

[alexander :surkov (:asurkov)]

are there any other stoppers than IsValid()?

Comment 15

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #14)
> are there any other stoppers than IsValid()?

that'd probably be enough to make me live, but I'd be happier with size_t and canonicalizing stuff too.

Comment 16

[alexander :surkov (:asurkov)]

Attachment: patch

switched to IsValid

Comment 17

[alexander :surkov (:asurkov)]

Trev, ping?

Comment 18

[Trevor Saunders (:tbsaunde)]

(In reply to alexander :surkov from comment #17)
> Trev, ping?

err I somehow missed there was another patch :(

Comment 19

[Trevor Saunders (:tbsaunde)]

Comment on attachment 8419410 [details] [diff] [review]
patch

>   nsIntRect CharBounds(int32_t aOffset, uint32_t aCoordType)
>-    { return TextBounds(aOffset, aOffset + 1, aCoordType); }
>+  {
>+    int32_t endOffset = aOffset == static_cast<int32_t>(CharacterCount()) ?
>+      aOffset : aOffset + 1;
>+    return TextBounds(aOffset, endOffset, aCoordType);
>+  }

not sure I see how this is related, but it seems reasonable.

Comment 20

[alexander :surkov (:asurkov)]

CharBounds(CharacterCount()) failed instead empty string as expected

Comment 21

[alexander :surkov (:asurkov)]

http://hg.mozilla.org/integration/mozilla-inbound/rev/885e99ddd324

Comment 22

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/885e99ddd324