Closed Bug 104651 Opened 23 years ago Closed 23 years ago

need two types of dependent string

Categories

(Core :: XPCOM, defect, P2)

Product:
Core
Component:
XPCOM
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla0.9.6

People

(Reporter: dbaron, Assigned: dbaron)

References

Details

Attachments

(1 file, 3 obsolete files)

single patch, works at least superficially on Linux, Mac OS9, Windows (Windows commercial too)
31.87 KB, patch
jag+mozilla
: review+
Details | Diff | Splinter Review 
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.
Assignee: scc → dbaron
Status: NEW → ASSIGNED
Priority: -- → P2
Target Milestone: --- → mozilla0.9.6
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
Attachment #56045 - Flags: review+
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
Attachment #56294 - Flags: review+
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.
Status: ASSIGNED → RESOLVED
Closed: 23 years ago
Resolution: --- → FIXED
Component: String → XPCOM
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: