http://lxr.mozilla.org/seamonkey/source/xpcom/string/public/nsAlgorithm.h#88 the loop looks like it may be a leftover from multi-fragment strings; since all strings are single-fragment now, it seems like this could now always be done in a single call to |write|. if that's done, it may be possible to make |write| return void, assuming it will always write all the data (but that may be a bad assumption?) One problem: It seems that nsScannerString still uses multi-fragment strings. Would that make this bug INVALID/WONTFIX?

Yes, you have to make sure that nsScannerString has its own overloaded version of copy_string, or you would want to rename and ensure that it is still using the multifragment algo. I think that most of the string stuff was forked for nsScannerString, but it might still be picking up things like copy_string. Someone should take a look at that.

this makes the following optimizations: 1) make copy_string work on a single fragment. 2) constify the |InputIterator& first| argument to copy_string and don't increment it, since no one in the tree actually cares. 3) make write return void, since memmove is predictable. 4) remove copy_string_backward, since there are no consumers. 5) provide a copy_multifragment_string for nsScannerString. 6) update a few functions in nsReadableUtils.cpp to singlefragmentedness. note that a side-effect of 1) is that copy_string will now call write even if first == last, but memmove will handle a zero-byte copy just fine, and we probably don't want the extra branch for this rare case. jst, i hope you don't mind if i tag you for this :)

jst pointed out that nsUTF8Utils.h also has some write()s - so i grepped around the tree a bit and found a bunch more. this fixes all of them. the only two consumers who looked at the rv, in toolkit/xre, can probably do fine without it - for instance, the converter's write() functions returned the entire requested length N even in failure cases, so it doesn't seem all that useful.

Comment on attachment 294916 [details] [diff] [review] patch v1 (diff -w) - In IsASCII() (nsReadableUtils.cpp): - // for each chunk of |aString|... - PRUint32 fragmentLength = 0; nsAString::const_iterator iter; - for ( aString.BeginReading(iter); iter != done_reading; iter.advance( PRInt32(fragmentLength) ) ) + for ( aString.BeginReading(iter); iter != done_reading; ++iter ) { - fragmentLength = PRUint32(iter.size_forward()); - const PRUnichar* c = iter.get(); - const PRUnichar* fragmentEnd = c + fragmentLength; - - // for each character in this chunk... - while ( c < fragmentEnd ) - if ( *c++ & NOT_ASCII ) + if ( *iter & NOT_ASCII ) return PR_FALSE; } This changes the inner-most loop of this code to loop over an iterator rather than simply looping over the PRUinchar*. Maybe a good compiler can generate identical code for the two cases, but I'd bet this would be faster if you got the PRUnichar pointer and the end out of the iterator upfront, and looped directly over that instead. Same thing in the other functions changed in the same file. - In parser/htmlparser/src/nsScannerString.cpp: +// private helper function +template <class InputIterator, class OutputIterator> +static inline +OutputIterator& +copy_multifragment_string( InputIterator& first, const InputIterator& last, OutputIterator& result ) + { This is only ever going to be used with one type of arguments right? If so, could we declare them here rather than using templates here? r- based on the first comments here, just since I think I should look that over once you've made those changes, but the rest look good to me.

Comment on attachment 295163 [details] [diff] [review] supplement class nsFileCharSink { public: typedef CharT value_type; public: nsFileCharSink( FILE* aOutputFile ) : mOutputFile(aOutputFile) { } - PRUint32 + void write( const value_type* s, PRUint32 n ) { - return fwrite(s, sizeof(CharT), n, mOutputFile); + fwrite(s, sizeof(CharT), n, mOutputFile); } This is a bit scary, as an error here would get lost (not that it wouldn't before this change). But yay for unused code! Wanna file a bug to remove nsFileCharSink while you're here? r+sr=jst

nice catch with the iterators - i didn't know they were still storing beginning, position, and end in the single-fragment world. :/ the nsReadableUtils changes are at the end of the diff.

Comment on attachment 295166 [details] [diff] [review] v2 (diff -w, whole thing) r+sr=jst!

checked in.