Closed Bug 1305422 Opened 3 years ago Closed 3 years ago

remove size_{forward,backward} and other similarly useless things from ns{Reading,Writing}Iterator

Categories

(Core :: XPCOM, defect, P3)

defect

Tracking

()

RESOLVED FIXED
mozilla52
Tracking Status
firefox52 --- fixed

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(20 files, 2 obsolete files)

1.35 KB, patch
erahm
: review+
Details | Diff | Splinter Review
4.18 KB, patch
erahm
: review+
Details | Diff | Splinter Review
1.17 KB, patch
erahm
: review+
Details | Diff | Splinter Review
1.15 KB, patch
erahm
: review+
Details | Diff | Splinter Review
1.56 KB, patch
baku
: review+
Details | Diff | Splinter Review
3.63 KB, patch
erahm
: review+
Details | Diff | Splinter Review
2.50 KB, patch
smaug
: review+
Details | Diff | Splinter Review
1.75 KB, patch
dholbert
: review+
Details | Diff | Splinter Review
1.42 KB, patch
erahm
: review+
Details | Diff | Splinter Review
1.20 KB, patch
baku
: review+
Details | Diff | Splinter Review
2.30 KB, patch
erahm
: review+
Details | Diff | Splinter Review
3.69 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
1.05 KB, patch
baku
: review+
Details | Diff | Splinter Review
1.22 KB, patch
bkelly
: review+
Details | Diff | Splinter Review
4.39 KB, patch
Details | Diff | Splinter Review
2.61 KB, patch
erahm
: review+
Details | Diff | Splinter Review
1.32 KB, patch
erahm
: review+
Details | Diff | Splinter Review
6.35 KB, patch
froydnj
: review+
mgoodwin
: review+
Details | Diff | Splinter Review
1.41 KB, patch
ted
: review+
Details | Diff | Splinter Review
3.13 KB, patch
erahm
: review+
Details | Diff | Splinter Review
ns{Reading,Writing}Iterator are strange beasts insofar as they know how much farther iteration can proceed before there is no more string to iterate over.  This behavior is very unusual, and there are even comments that this behavior stems from when we had fragmented strings and iterators to match.

ns{Reading,Writing}Iterator are also less powerful versions of mozilla::RangedPtr, so we should try to move them over to the more modern alternative.  Getting rid of size_{forward,backward} is a good step in that direction.
They are never called.
Attachment #8794830 - Flags: review?(erahm)
We have better ways of checking for SetLength failure nowadays, and even
if these SetLength calls did fail, the program would crash anyway.
Attachment #8794831 - Flags: review?(erahm)
Asking for size_forward on an iterator that you haven't started reading
from is just asking for the length of the original string, so use that
instead.
Attachment #8794832 - Flags: review?(erahm)
We don't need to bother with iterators here at all; we can just ask the
string for its data and length directly.
Attachment #8794833 - Flags: review?(erahm)
The implementation of Is8bit, with its multiply-nested loops, dates from
the time when string iterators could be fragmented into multiple pieces.
We no longer have such iterators, so we can write Is8bit much more
straightforwardly, with the single loop you would expect.
Attachment #8794834 - Flags: review?(amarchesini)
This change is necessary so we can start using a pair of
iterators (current, end) and subtract them to figure out how far we have
left to go.  The current code just uses size_forward and size_backward
for this purpose, and it's quite an unusual iterator that knows how far
it can go until it's done.
Attachment #8794835 - Flags: review?(erahm)
This change avoids calling size_forward on the iterator, which we are
trying to eliminate.
Attachment #8794836 - Flags: review?(erahm)
The implementation of SerializeAttr, with its multiply-nested loops,
dates from the time when string iterators could be fragmented into
multiple pieces.  We no longer have such iterators, so we can write
SerializeAttr much more straightforwardly.
Attachment #8794837 - Flags: review?(bugs)
The surrounding code in this file and elsewhere uses pairs of RangedPtrs
for iterators.  While we're trying to modify nsString's iterators so
they're more like RangedPtr, it seems reasonable to go ahead and modify
this instance of nsString iterators to use RangedPtr directly for
conformity with the surrounding code.
Attachment #8794838 - Flags: review?(dholbert)
These methods are now unused thanks to previous patches.
Attachment #8794839 - Flags: review?(erahm)
See comment 0 for the rationale on why we want to remove size_forward calls.
Attachment #8794840 - Flags: review?(amarchesini)
See comment 0 for the rationale on why we want to remove size_forward calls.
Attachment #8794841 - Flags: review?(erahm)
See comment 0 for the rationale on why we want to remove size_forward calls.
Attachment #8794842 - Flags: review?(bkelly)
See comment 0 for the rationale on why we want to remove size_forward calls.
Attachment #8794843 - Flags: review?(amarchesini)
See comment 0 for the rationale on why we want to remove size_forward calls.
Attachment #8794844 - Flags: review?(bkelly)
nsCString::const_iterator and friends currently hold pointers to not
only the current iterator position, but also the start and end of the
string.  This functionality enables them to compute their position from
the start and the end of the string, respectively, but we have better
ways of accomplishing the same thing (e.g. mozilla::RangedPtr), and it's
a bit unusual for iterators to know their current position anyway.  So
let's make nsPKCS12Blob, when reading digests, hold pointers to both the
current iterator position and the end iterator position, and calculate
the remaining amount to read from those two pieces of information.

I apologize in advance for modifying this grotty code.  I think I could have
written this patch more cleanly, but then it would have been mixed up between
changes and cleanup, and I think it would have been harder to review.
Attachment #8794845 - Flags: review?(dkeeler)
See comment 0 for the rationale on why we want to remove size_forward calls.
Attachment #8794846 - Flags: review?(ted)
We don't support IBM VisualAge for OS/2 anymore, and we haven't needed
this code in a long time regardless.
Attachment #8794848 - Flags: review?(erahm)
Attachment #8794834 - Flags: review?(amarchesini) → review+
Attachment #8794840 - Flags: review?(amarchesini) → review+
Attachment #8794843 - Flags: review?(amarchesini) → review+
Assignee: nobody → nfroyd
Status: NEW → ASSIGNED
Attachment #8794830 - Flags: review?(erahm) → review+
Attachment #8794831 - Flags: review?(erahm) → review+
Attachment #8794832 - Flags: review?(erahm) → review+
Comment on attachment 8794845 [details] [diff] [review]
part 15 - don't call size_forward in nsPKCS12Blob

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

I would actually prefer to remove the code that calls this entirely since it's completely unnecessary (NSS has this functionality built-in). I wanted to prove to myself that this would work, so I wound up just writing the patch. I'll attach it shortly.
Attachment #8794845 - Flags: review?(dkeeler)
Attachment #8794833 - Flags: review?(erahm) → review+
Comment on attachment 8794928 [details] [diff] [review]
part 15 - don't call size_forward in nsPKCS12Blob (code removal version)

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

Fantastic, thank you!
Attachment #8794928 - Flags: review?(nfroyd) → review+
Comment on attachment 8794835 [details] [diff] [review]
part 6a - add operator- support for ns{Reading,Writing}Iterator

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

::: xpcom/string/nsStringIterator.h
@@ +131,5 @@
>    }
> +
> +  difference_type operator-(const self_type& aOther)
> +  {
> +    MOZ_ASSERT(mPosition >= aOther.mPosition);

I'm not sure we should be asserting here. We return |difference_type| (aka ptrdiff_t) which is signed, so that implies we allow for negative distances.

I *could* see an argument for asserting that the iterators are working on the same string, something like
MOZ_ASSERT(mStart == aOther.mStart && mEnd == aOther.mEnd) but that seems a bit gratuitous and I'm guessing we want to get rid of mStart and mEnd anyhow.

@@ +267,5 @@
>    }
> +
> +  difference_type operator-(const self_type& aOther)
> +  {
> +    MOZ_ASSERT(mPosition >= aOther.mPosition);

ditto
Attachment #8794835 - Flags: review?(erahm) → review-
Attachment #8794836 - Flags: review?(erahm) → review+
Attachment #8794839 - Flags: review?(erahm) → review+
Attachment #8794841 - Flags: review?(erahm) → review+
Comment on attachment 8794847 [details] [diff] [review]
part 17 - remove size_{forward,backward} from ns{Reading,Writing}Iterator

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

\o/
Attachment #8794847 - Flags: review?(erahm) → review+
Attachment #8794848 - Flags: review?(erahm) → review+
Didn't code this up right the first time, let's try again.
Attachment #8794996 - Flags: review?(ted)
Attachment #8794846 - Attachment is obsolete: true
Attachment #8794846 - Flags: review?(ted)
Attachment #8794837 - Flags: review?(bugs) → review+
Version 2 of this patch adds some rationale for why we do what we do and
changes the result type of operator-.
Attachment #8795018 - Flags: review?(erahm)
Attachment #8794835 - Attachment is obsolete: true
Comment on attachment 8795018 [details] [diff] [review]
part 6a - add operator- support for ns{Reading,Writing}Iterator

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

Looks good, you can also get rid of the uint32_t casts in part 6 now I think.
Attachment #8795018 - Flags: review?(erahm) → review+
Attachment #8794928 - Flags: review?(mgoodwin) → review+
Comment on attachment 8794996 [details] [diff] [review]
part 16 - don't call size_backward in nsExceptionHandler.cpp

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

I don't think we actually have a test for this, that might be useful. You could shoehorn it in here:
https://dxr.mozilla.org/mozilla-central/rev/c55bcb7c777ea09431b4d16903ed079ae5632648/toolkit/crashreporter/test/unit/test_crashreporter_crash.js#34

We have tests for bad chars in keys, but not for the escaping we do for values:
https://dxr.mozilla.org/mozilla-central/rev/c55bcb7c777ea09431b4d16903ed079ae5632648/toolkit/crashreporter/test/unit/test_crashreporter.js#47

Unrelated, but I wish I had just made the .extra file JSON when I originally wrote this code.
Attachment #8794996 - Flags: review?(ted) → review+
Attachment #8794842 - Flags: review?(bkelly) → review+
Attachment #8794844 - Flags: review?(bkelly) → review+
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/77a8d7d6a0fb
part 1 - remove normalize* methods from ns{Reading,Writing}Iterator; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/8313868b4334
part 2 - don't call size_forward on ns*String::iterators to check for SetLength failure; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/e208dfe24c94
part 3 - don't call size_forward in nsTStringComparator.cpp; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/a5b04e48b3e8
part 4 - don't call size_forward in nsIDNService.cpp; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/1c06cb170aa8
part 5 - simplify Is8bit; r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/9b814ee26cd7
part 6a - add operator- support for ns{Reading,Writing}Iterator; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/9dc21248ab12
part 6 - pass explicit end iterators for CopyTo{Upper,Lower}Case; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/caad2bb558f6
part 7 - simplify nsXMLContentSerializer::SerializeAttr; r=smaug
https://hg.mozilla.org/integration/mozilla-inbound/rev/8c6e2e1c4f2a
part 8 - make nsSMILParserUtils::CheckForNegativeNumber more idiomatic; r=dholbert
https://hg.mozilla.org/integration/mozilla-inbound/rev/55be43ac96c9
part 9 - remove {start,end} from ns{Reading,Writing}Iterator; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/32b8ba9f7a51
part 10 - don't call size_forward in nsHTMLContentSerializer.cpp; r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/088bfcd8f3cb
part 11 - don't call size_forward in nsXPCOMStrings.cpp; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/17a8607397e5
part 12 - don't call size_forward in {Body,Fetch}Util.cpp; r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/b26623101a0d
part 13 - don't call size_forward in nsXMLContentSerializer.cpp; r=baku
https://hg.mozilla.org/integration/mozilla-inbound/rev/e8f15fc02cfc
part 14 - don't call size_forward in nsDefaultURIFixup.cpp; r=bkelly
https://hg.mozilla.org/integration/mozilla-inbound/rev/daf738c4a411
part 15 - don't call size_forward in nsPKCS12Blob; r=mgoodwin,nfroyd
https://hg.mozilla.org/integration/mozilla-inbound/rev/8ad96d4c9d42
part 16 - don't call size_backward in nsExceptionHandler.cpp; r=ted.mielczarek
https://hg.mozilla.org/integration/mozilla-inbound/rev/f933f6215b9d
part 17 - remove size_{forward,backward} from ns{Reading,Writing}Iterator; r=erahm
https://hg.mozilla.org/integration/mozilla-inbound/rev/c8c685a95ab0
part 18 - remove #if 0'd-out code from nsStringIterator.h; r=erahm
Pushed by nfroyd@mozilla.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/3caff20982f8
followup - use correct check for memory allocation failure in nsWindowsRegKey.cpp; r=bustage
You need to log in before you can comment on or make changes to this bug.