Right now nsDependentString inherits from nsAFlatString, but there are some users that pass it a string that is not flat, by initializing it with a length (e.g., nsAString::do_AssignFromElementPtrLength) -- and I think it would also be useful in other places -- e.g., nsLocalFile(Unix)::InitWithPath. There should be two types of nsDependentString (I'm not sure how the names should work): 1) One that inherits from nsAFlatString, and asserts in its constructor that takes a length that the length given is the real length. 2) One that inherits from nsASingleFragmentString. Once such classes exist we should go through existing users and make sure they are using the right one.

Another non-flat use of nsDependentString is in nsMetaCharsetObserver.cpp, checked in as part of attachment 53020 [details] [diff] [review] on bug 100214.

One solution to the naming would be to make an nsDependentSubstring that inherits from nsASingleFragmentString and rename the current nsDependentSubstring to nsDependentMFSubstring, nsDependentSubAString, or something...

I'm confused - is this bug because FlatStrings are supposed to be null-terminated, but sometimes we initialize them with a length? (i.e. by "non-flat" do you mean "not null terminated"?)

nsAFlatString requires null-termination, and thus has a |get| method. nsASingleFragmentString is going to be put into the hierarchy and will not have a |get| method. Right now, nsDependentString is assumed to be flat, and should probably have an assertion in its constructor that (ptr[length] == '\0'). Another possibility could be using nsDependentString inheriting from nsASingleFragmentString and nsLiteralString inheriting from nsAFlatString. We want the initialization-with-length as a performance enhancement for when we already know the length (which is used at least by NS_LITERAL_STRING, and also in other places).

OK, here's another proposal, which I like better than my previous one: We want char_type* constructors for the dependent strings that inherit from nsAFlatString and nsASingleFragmentString, so we want them to have nice, memorable names. I think good names for these two strings would be nsDependentString and nsDependentSubstring. The current nsDependentSubstring is not intended to be used from code -- it's only the temporary returned from |Substring|. However, in the world of nsASingleFragmentString, we would want a second |Substring| function that takes an nsASingleFragmentString and returns an nsASingleFragmentString. This suggests that nsDependentSubstring isn't all that bad a name after all -- it would be the return type of the |Substring| function on single fragment strings. So, in other words, I'm proposing we have following types and functions (with much omitted): nsDependentString : public nsAFlatString { // All three constructors assert to ensure non-NULL-ness nsDependentString(const char_type*); // The following two constructors assert to ensure flat-ness nsDependentString(const char_type*, const char_type*) nsDependentString(const char_type*, size_type); }; nsDependentSubstring : public nsASingleFragmentString { // All three constructors assert to ensure non-NULL-ness nsDependentString(const char_type*); nsDependentString(const char_type*, const char_type*) nsDependentString(const char_type*, size_type); }; [ for lack of a better name that I can think of now ] nsDependentSubAString : public nsA[Promise]String { [ existing constructors of nsDependentSubstring ] }; inline const nsDependentSubAString Substring( const nsAString::const_iterator& aStart, const nsAString::const_iterator& aEnd ) { return nsDependentSubAString(aStart, aEnd); } inline const nsDependentSubAString Substring( const nsAString& aString, PRUint32 aStartPos, PRUint32 aSubstringLength ) { return nsDependentSubAString(aString, aStartPos, aSubstringLength); } inline const nsDependentString Substring( const nsASingleFragmentString::const_iterator& aStart, const nsASingleFragmentString::const_iterator& aEnd ) { return nsDependentSubstring(aStart, aEnd); } inline const nsDependentString Substring( const nsASingleFragmentString& aString, PRUint32 aStartPos, PRUint32 aSubstringLength ) { return nsDependentSubstring(aString.[???] + aStartPos, aSubstringLength); }

OK, we discusssed this: We should just have nsSingleFragmentSubstring that inherits from nsASingleFragmentString and be constructed using |Substring|.

er, nsDependentSingleFragmentSubstring.

Taking.

Er, ignore the diff to configure.

When reviewing that patch, it will make a lot more sense to compare it to attachment 53020 [details] [diff] [review] in bug 100214, since many of the changes outside of the string directory are correcting mistakes in that patch.

Comment on attachment 56045 [details] [diff] [review] patch >Index: mozilla/string/public/nsDependentSubstring.h >=================================================================== >RCS file: /cvsroot/mozilla/string/public/nsDependentSubstring.h,v >retrieving revision 1.6 >diff -u -d -r1.6 nsDependentSubstring.h >--- nsDependentSubstring.h 2001/10/13 15:01:17 1.6 >+++ nsDependentSubstring.h 2001/11/01 05:32:02 >@@ -39,7 +43,7 @@ > // > > class NS_COM nsDependentSubstring >- : public nsAPromiseString >+ : public nsAString > /* > NOT FOR USE BY HUMANS (mostly) > >@@ -92,7 +96,7 @@ > }; > > class NS_COM nsDependentCSubstring >- : public nsAPromiseCString >+ : public nsACString > /* > NOT FOR USE BY HUMANS (mostly) > I don't think you want to change that until we've fixed Assign, Append, Insert etc. to all check for |IsDependentOn|. >@@ -145,11 +149,91 @@ > }; > > >+class NS_COM nsDependentSingleFragmentSubstring >+ : public nsASingleFragmentString >+ { ... >+ void >+ Rebind( const abstract_single_fragment_type& aString, const PRUint32 aStartPos, const PRUint32 aLength ) >+ { >+ const char_type* iter; I guess I'd prefer |const_char_iterator iter;| >+ mHandle.DataStart(aString.BeginReading(iter) + NS_MIN(aStartPos, aString.Length())); >+ mHandle.DataEnd( NS_MIN(aString.EndReading(iter), mHandle.DataStart() + aLength) ); Hmmm, this one took a while to parse. I think it would be more understandable as: mHandle.DataEnd( NS_MIN(mHandle.DataStart() + aLength, aString.EndReading(iter)) ); But maybe that's just me :-) r=jag

Comment on attachment 56045 [details] [diff] [review] patch I don't really care if you fix the "mistakes" from bug 100214, but I don't understand how they were mistakes.. I never used .get(), and I was passing in to the string library's own Compare() function, so I don't understand what was wrong with the existing code. Is this going to now start making copies of that substring?

> I don't really care if you fix the "mistakes" from bug 100214, but I don't > understand how they were mistakes.. I never used .get(), and I was passing > in to the string library's own Compare() function, > so I don't understand what was wrong with the existing code. There were some places where you forgot to do any substrings when replacing strncasecmp (e.g., the one that caused bug 105459), and other places where you used nsDependentString when it was technically inappropriate (and this patch makes it assert). > Is this going to now start making copies of that substring? No. It merely ensures that a string that is not null-terminated is never claims it is a |nsAFlatString| (which is a single-fragment, null-terminated, string).

> I don't think you want to change that until we've fixed Assign, Append, Insert > etc. to all check for |IsDependentOn|. Hmmm. So my new string will inherit from nsASingleFragmentString. This doesn't seem to be a problem right now, and any current dependency stuff using nsAPromiseString won't work when a promise is passed through as nsAString (which is the problem IsDependentOn is solving). But I guess I'll leave it for now...

Comment on attachment 56294 [details] [diff] [review] single patch, works at least superficially on Linux, Mac OS9, Windows (Windows commercial too) r=jag

Comment on attachment 56294 [details] [diff] [review] single patch, works at least superficially on Linux, Mac OS9, Windows (Windows commercial too) sr=scc

Fix checked in 2001-11-06 20:12 PDT, etc.