75220 - [API] fix string names

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Scott Collins]

Waterson points out that `local' strings don't have to be local and the name
isn't as helpful as he would like.  Shaver and jag have the same attitude
towards `promises'.

  nsPromise[C]Subtring      --> nsDependent[C]Substring
  nsLocal[C]String          --> nsDependent[C]String
  nsPromise[C]Concatenation --> nsDependent[C]Concatenation

  nsCommon[C]String         --> nsSharable[C]String

|nsPromiseFlat[C]String| is a good name that we should keep, I think.  It's more
complicated than a simple dependency, since it may copy.

It's still a question how the string converter classes fit into this.

Comment 1

[Axel Hecht]

I just read the string_iterator doc, and the Capitalize example triggered a 
name discussion in my head:
The sample changes the fed string in place, but
copy_string(S.BeginWriting(), S.EndWriting(), Capitalize());
sounds like Capitalize() couldn't change S to me.
How about feed_string? iterate_string?

Axel

Comment 2

[Scott Collins]

These are the 0.9 bugs that didn't make it, and that I intend to fix in 0.9.1

Comment 3

[Scott Collins]

  Promises --> IsDependentOn    

Comment 4

[Scott Collins]

re-targeting milestones, starting from a clean slate

Comment 5

[Scott Collins]

All planned string API fixes need to be in by mozilla1.0.

Comment 6

[Brendan Eich [:brendan]]

I agree dependent comes to mind due to a direct data dependency, but Wrapped is
shorter and sweeter IMO for at least what is currently nsLocal[C]String.  Or am
I weird?

/be

Comment 7

[Peter "jag" Annema]

nsDependent [C]String
nsWrapped   [C]String
nsWrapper   [C]String
nsWrapping  [C]String

nsWrapper has my preference as a replacement for what's currently nsLocal.

Especially if the new nsLocal[C]String will double as what currently is
nsXPIDL[C]String, "dependent" won't apply in the |getter_Copies| case.

Comment 8

[Scott Collins]

Re: wrapped

But it's not wrapped or wrapping or a wrapper.  The only string that comes close
to that definition, in my mind, is |nsPromiseFlat[C]String| which keeps a
reference to another string, and forwards calls through it.  The closest name to
the real concept of a dependent string is `non-owning'.  Negatives in
identifiers are usually problematic.  A dependent string managing access to a
local stack-based buffer is no more a wrapper than the longer-lived heap-based
string is a wrapper for _its_ buffer.  If this were the case, all strings could
be considered `wrappers'.  In both cases, the string in question might well be
the only string pointing at that buffer.  One asserts ownership, the other
doesn't.  The key information to communicate to clients is that a dependent
string has a dependency on data that the client must, themselves, manage, e.g.,
by controlling the lifetime with suitable declarations.  Dependent strings are
dependent on somebody else's data.  They are dependent on the client ensuring
that the data in question stays around for the useful life of the dependent
string.  To my, perhaps jaded, eye, `wrappe[rd]' doesn't convey this information.


Re: |nsXPIDL[C]String|

In the |getter_Copies| case, the destination string must accept ownership, so
this will be some transformation that results in

  nsSharableString /* nee |nsCommonString| */::rebind( ptr )

The |getter_Shares| case must make a dependent string

  nsDependentString /* nee |nsLocalString| */::rebind( ptr )

This change is not going to end up being transparent.  It's not worth making a
class that alternatively acquires ownership for the eleven instances of
|getter_Shares| in the tree

  http://lxr.mozilla.org/seamonkey/search?string=getter_Shares

In my opinion, it's better to keep a clear distinction.  Instances of
such-and-such a class _always_ own.  Instances of such-and-such _never_ own.
Admittedly, |nsPromiseFlat[C]String| conceptually breaks this guideline already
(though not physically).

Comment 9

[Scott Collins]

The easy path to this starts with some file _copying_ in the cvs tree

  string/public/nsPromiseSubstring.h,v     --> nsDependentSubstring.h,v
                nsLocalString.h,v          --> nsDependentString.h,v
                nsPromiseConcatenation.h,v --> nsDependentConcatenation.h,v
                nsCommonString.h,v         --> nsSharableString.h,v

        /src/nsPromiseSubstring.cpp,v      --> nsDependentSubstring.cpp,v
             nsLocalString.cpp,v           --> nsDependentString.cpp,v
             nsPromiseConcatenation.cpp,v  --> nsDependentConcatenation.cpp,v
             nsCommonString.cpp,v          --> nsSharableString.cpp,v

Copying for now.  I'll fix the names within the new files, then add them to the
build and export, then make the rest of the tree use the new names, then remove
the old files (with appropriate reviews at the appropriate stages).

Comment 10

[Daniel (Leaf) Nunes]

your files have been so copied. mekkalekkahai, mekkahaineeho.

Comment 11

[Scott Collins]

Attachment: use |nsSharable[C]String| instead of |nsCommon[C]String| everywhere

Comment 12

[Scott Collins]

reviews requested of jst and vidur on the first patch attached above, to unblock
my work in bug #74726 (email sent, and cc'ing [e-mail]).

Comment 13

[Johnny Stenback (:jst)]

r/sr=jst

Comment 14

[Dan Rosen]

So the decision on #mozilla, given Mirriam-Webster's friendly input, is that
these should be called |nsShareable[C]String| ("e" added). Once you have the
files in CVS renamed, you should be able to automate the rest of the renaming
process with a little unix command-line ingenuity (my guess is some combination
of sed and xargs will ease the pain). r=dr, conditional on the name change.

Comment 15

[Dan Rosen]

Nominating for 0.9.2, while I'm at it. The sooner APIs can stabilize, the
better... I'm hoping the same process of converting Common to Shareable is
quickly repeatable for the other naming changes :)

Comment 16

[Scott Collins]

Attachment: fix spelling, add new files with correctly spelled name

Comment 17

[Dan Rosen]

scc: That patch is broken. It uses "Shareable" in some places and "Sharable" in
others.

Comment 18

[Mike Shaver (:shaver emeritus)]

scc, you acquiesce too quickly.

``sharable'' is a perfectly valid spelling, and dictionary.com agrees, though
Mr. Webster's revisionist tendencies might have made him loath to accept it.

Perhaps more importantly, we already have nsAWritableString, and choosing the
elided-e form for one class, and the retained-e form for another is sure to
permanently confuse our spelling-challenged colleagues.

scc, please use ``nsSharable[C]String''.

(By the way, Dan, it's ``Merriam-Webster''.)

Comment 19

[Dan Rosen]

> ``sharable'' is a perfectly valid spelling, and dictionary.com agrees, though
> Mr. Webster's revisionist tendencies might have made him loath to accept it.

m-w.com places "sharable" as an alternative sharing of "shareable", and it was
dbaron's opinion on irc that the former was "funny-looking." However...

> Perhaps more importantly, we already have nsAWritableString, and choosing the
> elided-e form for one class, and the retained-e form for another is sure to
> permanently confuse our spelling-challenged colleagues.

I'll defer to this argument. Using "sharable" makes a lot more sense in the
context of consistency with prior naming.

scc: You have my r= on the first patch, unmodified. Sorry to give you trouble here.

Comment 20

[Scott Collins]

Patch changing |nsCommon[C]String| --> |nsSharable[C]String| checked in.

Comment 21

[Scott Collins]

Attachment: fixes `dependent' names, exports headers, builds implementations

Comment 22

[Scott Collins]

The patch above fixes the names in the new files, adds them to the build and
exports the appropriate headers.  After that patch is checked in, a further
patch will typedef the old names into aliases for the new; remove the old
implementations from the build; and fix all users of the old names; and finally
cvs remove the old files.  This first patch can be checked in safely before
those changes though, and is worth reviewing separately.

Comment 23

[Axel Hecht]

r=[e-mail]

Comment 24

[vidur (gone)]

sr=vidur. Remember to get rid of the #if 0 in nsDependentConcatenation.h
 for the two global operators once you do the typedefing and remove the old
implementations from the build.

Comment 25

[vidur (gone)]

Also remember to remove the direct include of nsDependConcatentation.h from
nsDependConcatentation.cpp when the rest of the fixup is complete. Why the
ifndef around the include right now? 

+#ifndef nsDependentConcatenation_h___
+#include "nsDependentConcatenation.h"
+#endif

Comment 26

[Scott Collins]

Attachment: this covers the bulk of the tree, other than extensions (soap needs fixing still)

Comment 27

[Scott Collins]

The above patch fixes everything in the mozilla tree except soap.  I'll generate
a patch for that on my windows box.  Need to coordinate with the commercial tree
before we can check this stuff in.

Comment 28

[Scott Collins]

I can check in the patch above at will (i.e., not coordinating with commercial
or soap) if I also add the following |typedef|s in "nsLiteralString.h" and
"nsDependentString.h" respectively

  typedef nsDependentString  nsLiteralString;
  typedef nsDependentCString nsLiteralCString;


  typedef nsDependentString  nsLocalString;
  typedef nsDependentCString nsLocalCString;

with appropriate comments about their `temporary-ness'.  Then we can fix
commercial and soap separately, and finally remove these typedefs.  Review for
the patch above should be considered in this light.  Given the size of this
patch (~100k) I'll send email to three reviewers hoping for two reviews :-)

Comment 29

[Scott Collins]

Attachment: previous patch + |typedef|s to protect commercial and soap

Comment 30

[Johnny Stenback (:jst)]

Index: embedding/browser/powerplant/source/UMacUnicode.cpp
===================================================================
RCS file: /cvsroot/mozilla/embedding/browser/powerplant/source/UMacUnicode.cpp,v
retrieving revision 1.5
diff -u -2 -r1.5 UMacUnicode.cpp
--- UMacUnicode.cpp	2001/03/02 09:24:53	1.5
+++ UMacUnicode.cpp	2001/05/18 22:11:59
@@ -233,4 +233,4 @@
     memcpy(charBuf, &aIn[1], aIn[0]);
     charBuf[aIn[0]] = '\0';
-    return PlatformToUCS(nsLiteralCString(charBuf), aOut);
+    return PlatformToUCS(nsDependentCString(charBuf, aIn[0]), aOut);
 }

Looks really weird, but I think it's ok, except that it's doing one unecessary
memcpy, or did I misunderstand that code? Either way, I'm ok with this change,
just thought I'd mention it.

Other than that the changes look good to me, r/sr=jst

Comment 31

[Scott Collins]

Attachment: fixes uses of |nsLiteralC?String| in soap

Comment 32

[Scott Collins]

Attachment: patch to remove |nsLiteralC?String| and |nsLocalC?String| |typedef|s, after we've cleaned up all callers

Comment 33

[Scott Collins]

Attachment: rename member function |Promises| to |IsDependentOn|

Comment 34

[vidur (gone)]

r=vidur for the last 3 patches. The SOAP module is currently not part of the
standard build, though fixing it up makes sense if others are building it. Note
that a new version of the module is being worked on on a branch.

Comment 35

[Johnny Stenback (:jst)]

r/sr=jst for the changes, please note that the commercial tree still uses
nsLiteralString (~30 places), so those need to be fixed before we can remove the
typedef's completely.

Comment 36

[Scott Collins]

soap and |IsDependentOn| patches checked in; still need to fix commercial before
I can check in the patch that removes the |typedef|s (05/21/01 05:40).

Comment 37

[Scott Collins]

Attachment: updated patch to remove the |typedef|s and fix stragglers that have been checked in recently

Comment 38

[Scott Collins]

This latest patch (05/22/01 05:20) updates the earlier patch that removed the
|typedef|s for |nsLiteralC?String| and |nsLocalC?String|.  The updated version
fixes stragglers that have been checked in since.  mscott is holding the
matching commercial fixes in his local tree till the commercial tree opens.
After he checks those in, I can land this final patch and close this bug.

Comment 39

[Scott Collins]

final patches checked in.  I'll CVS remove the unused files anon.