Remove XPCOM dependency on string/obsolete

RESOLVED WONTFIX

Status

()

Core
XPCOM
P2
normal
RESOLVED WONTFIX
17 years ago
16 years ago

People

(Reporter: Jon Smirl, Assigned: Alec Flett)

Tracking

Trunk
mozilla1.2alpha
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

(Reporter)

Description

17 years ago
This is a first pass at removing XPCOM's dependency on the string/obsolete 
directory. I could fix everything except nsRegistry.cpp which needs UTF8 
conversions which are not available in the new string code.

This code compiles on Windows but I have not tried testing it yet. It's a 2,200 
line diff which is going to need to be carefully checked. There were also a few 
places where APIs needed to be changed to remove usage of the old strings. No 
attempt was made to check other platforms, they probably won't even compile.

95% of the changes were straight forward but a few were more complicated like 
Trim() which does not exist in the new string code.
(Reporter)

Comment 1

17 years ago
Created attachment 48916 [details] [diff] [review]
patches to remove string/obsolete dependency from XPCOM

Comment 2

17 years ago
kool!  Lets ask Jag to take a look at this patch.  He is much more skilled with
our string classes than I am.

scott, jag, is there any way to do  UTF8 conversions without string/obsolete?

Keywords: patch

Comment 3

17 years ago
Jon, could you attach a diff -u version of this patch for context and
readability?
(Reporter)

Comment 4

17 years ago
Created attachment 48930 [details] [diff] [review]
-u version
I'm surprised that nsAFlatString doesn't have any pure virtual methods. 
However, I think it was still intended as an abstract type, whereas
nsDependentString and nsSharableString are not.

What is the goal of removing this dependency?  I think at some point in the
future |nsString| will have a new implementation (based on the current
nsSharableString?) that lives in string/public and string/src (although probably
with a bunch of methods removed / modified).

I also thought scc was rewriting the converters as generators a while ago...

scc?

Comment 6

17 years ago
Collided with dbaron who said what I wanted to say, but here's my comment
anyway:

This won't work. Even though the code compiles, the |A| in nsAFlatString and
nsAString stands for abstract, meaning that you can't directly use the class.
What's lacking in this case is a mechanism for allocating memory when assigning
into it.

You should be able to use nsSharableString for this purpose though.
(Reporter)

Comment 7

17 years ago
Does nsSharableString incur a locking/refcount overhead? I can switch if this 
isn't going to be a performance issue. If I remember the changes right a global 
replace should work.

I just had a bad experience with Xalan doing multi-thread locks on strings and 
ruining performance.

I wish there was better documentation for the string code. I find it entirely 
confusing as to what to use.  I've got another entire project that can't even 
compile because of string changes.
(Reporter)

Comment 8

17 years ago
Created attachment 48981 [details] [diff] [review]
-u with Flat changed to Sharable
(Reporter)

Comment 9

17 years ago
Created attachment 48990 [details] [diff] [review]
Made it build debug/non-debug on Windows

Comment 10

17 years ago
Let me first comment that ns[C][Auto]String are not obsolete, even though they
currently are in that directory. They will become versions of
ns[C]SharableString. The changes you are making don't really make sense unless
there's an urgent need to get rid of some build dependency that can't wait till
the strings have been cleaned up.

I also see that in a lot of places you have converted nsString& parameters to
nsAString& parameters. While this is laudable, is it what we really want? In
some cases we might want nsAFlatString& for performance reasons (where we know
the string will be flat, i.e. single fragment, zero-terminated, and most or all
usage of that code will be from flat strings, and we can hand flatten the few
non flat strings).

Some comments on your patch as it is:

+    nsSharableCString str; 
+       CopyUCS2toASCII(nsDependentString(message), str);

Fix your editor to not insert hard tabs.

+          status = (*_retval = nsCRT::strdup((*entry).get())) ? NS_OK :
NS_ERROR_OUT_OF_MEMORY;

Perhaps you could use ToNewCString(*entry) there.

-    return AddSubtree( baseKey, NS_ConvertUCS2toUTF8(keyname).get(), _retval );
+       nsSharableCString name;
+       CopyUCS2toASCII(nsDependentString(keyname), name);

This is not the same thing. There is no way to currently do what you want.

-    nsAutoString tmp;
+    nsSharableString tmp;

This is not the same thing. nsAutoString provides a 64 unit buffer on the stack,
which generally makes it cheaper than an nsString for use with short strings.
nsString will always need to allocate memory, where nsAutoString will only need
to allocate when it needs more than 64 units.

-[ref] native nsStringRef(nsAWritableString);
+[ref] native nsStringRef(nsAString);

Hmmm... The other part of this is:

[noscript] void ToString(in nsStringRef aString); 
 
Do you think people would mind if that were changed to:

AString ToString();


------------


+                       for (int i = key.Length(); i > 0; i++) {
+                               PRUnichar ch = key.CharAt(i);
+                               if ((ch == L' ') || (ch == L'\t'))
+                                       key.Truncate(i - 1);
+                               else
+                                       break;
+                       }

I don't think L'x' is portable, use PRUnichar('x');
That should be i--, not i++, and CharAt is 0 based, so you'll not get the char
you want there.

How about something like:
PRInt32 i;
for (i = key.Length() - 1; i >= 0; --i) {
  PRUnichar ch = key.CharAt(i);
  if (ch != PRUnichar(' ') && ch != PRUnichar('\t'))
    break;
}
if (i != key.Length() - 1)
  key.Truncate(i);

---

and then a bit later, you do:


-      value.Trim(trimThese, PR_TRUE, PR_TRUE);
-      nsAutoString oldValue;
+                       for (i = value.Length(); i > 0; i++) {
+                               PRUnichar ch = value.CharAt(i);
+                               if ((ch == L' ') || (ch == L'\t'))
+                                       value.Truncate(i - 1);
+                               else
+                                       break;
+                       }
+      nsSharableString oldValue;
       mSubclass->SetStringProperty(key, value, oldValue);

The same mistakes as above, and this code was trimming from both ends.
This should work:

nsAString::const_iterator begin, end;
key.BeginReading(begin);
key.EndReading(end);
while (begin != end && (*begin == ' ' || *begin == '\t'))
  ++begin;

while (end != begin) {
  --end;
  if (*end != ' ' && *end != '\t')
    break;
}

nsSharableString oldValue;
mSubclass->SetStringProperty(key, Substring(begin, end), oldValue);

Btw, something similar to the above while loop could also be used for the first
case.

        PRUnichar test[]={0x0041,0x0051,0x0052,0x0000};
-       nsString T(test);
+       nsAString T(test);

This code should really fail to compile, since again nsAString is abstract and
should not be instantiated. Use |nsDependentString T(test);|. You use this
replacement pattern in a few places, btw, which will all need fixing.

Comment 11

17 years ago
alec is doing some work here.  Alec, care to own this one too.  
Assignee: dougt → alecf
Target Milestone: --- → mozilla1.0
(Reporter)

Comment 12

17 years ago
Who responsibile for finishing sorting out string vs string/obsolete? That work 
has to be finished before XPCOM can be adjusted to not use string/obsolete.

For example autostring is in string/obsolete when it should be in string. 
USC support is incomplete in string
The classes in string are missing some commonly needed helper methods.
The documentation for string is partial and needs a lot of help.
.....

Comment 13

17 years ago
Bugs targeted at mozilla1.0 without the mozilla1.0 keyword moved to mozilla1.0.1 
(you can query for this string to delete spam or retrieve the list of bugs I've 
moved)
Target Milestone: mozilla1.0 → mozilla1.0.1
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Priority: -- → P1
Target Milestone: mozilla1.0.1 → mozilla0.9.9
(Assignee)

Updated

17 years ago
Priority: P1 → P2
Target Milestone: mozilla0.9.9 → mozilla1.2
(Assignee)

Comment 14

16 years ago
not sure this is ever going to happen. string/obsolete isn't really obsolete -
its just the internal implementations of nsString and friends that need to be
cleaned up. marking WONTFIX.
Status: ASSIGNED → RESOLVED
Last Resolved: 16 years ago
Resolution: --- → WONTFIX
You need to log in before you can comment on or make changes to this bug.