334038 - nsStringUtils.h

Status: RESOLVED FIXED

(Core :: XPCOM, defect)

Description

[Benjamin Smedberg]

While porting code over to frozen linkage (camino, in this case), I keep finding complex behaviors in the old string code that I need to emulate. Rather than copy code around constantly, I want to unify it in nsStringUtils. I'm going to keep adding patches to this bug and leave it open as needed.

Comment 1

[Benjamin Smedberg]

Attachment: nsStringUtils

Summary:

Various functions in nsCRT that might be useful to frozen-linkage code have been moved to nsCRTGlue

I have introduced a new type nsStringContainer_Size in nsStringAPI.h which declares the size of the structure that must be allocated for calls to NS_StringContainerInit

I have moved all the C++ classes and declarations from nsStringAPI.h to nsStringUtils.h, and moved some of the more expensive operations to nsStringUtils.cpp.

Comment 2

[Benjamin Smedberg]

Oh, and this patch requires a small set of treewide s/nsStringAPI.h/nsStringUtils.h/ changes, which I didn't include in the patch because of the monotony it introduced ;-)

Comment 3

[Darin Fisher]

Comment on attachment 219344 [details] [diff] [review]
nsStringUtils

I'm not grasping why nsStringContainer_Size is necessary.  It seems that you are requiring nsStringUtils.h to be included in order to use the C API.  So, why can't you just define nsStringContainer to hold the member variables?

Comment 4

[Benjamin Smedberg]

I wanted to specify the necessary struct size without declaring nsStringContainer itself. Code that doesn't want the nsStringUtils.h dependency (like dlldeps.cpp) can just declare "class nsStringConntainer : private nsStringContainer_Size" or write their own C++ wrapper.

Comment 5

[Christian :Biesinger (don't email me, ping me on IRC)]

(In reply to comment #2)
> Oh, and this patch requires a small set of treewide
> s/nsStringAPI.h/nsStringUtils.h/ changes, which I didn't include in the patch
> because of the monotony it introduced ;-)

Does this mean that extensions that use nsStringAPI.h will now fail to compile?

Comment 6

[Benjamin Smedberg]

> Does this mean that extensions that use nsStringAPI.h will now fail to compile?

Yes. We guarantee binary compatibility but not source compat, and declaring only the C exports in the stringapi header is a good thing if astring/string classes are going to require a glue lib.

Comment 7

[Darin Fisher]

Comment on attachment 219344 [details] [diff] [review]
nsStringUtils

>Index: xpcom/ds/nsCRT.h

>+  static PRUint32 strlen(const PRUnichar* s) {
>+    return NS_strlen(s);
>+  }
...
>+  static char ToUpper(char aChar) { return NS_ToUpper(aChar); }

Would be nice to use the same bracket style for all nsCRT methods
that are defined inline.


>Index: xpcom/glue/Makefile.in
...
>+SDK_HEADERS = \
> 		pldhash.h \
> 		nsArrayUtils.h \

Excellent!


>Index: xpcom/glue/nsCRTGlue.h

>+#define FF '\f'
>+#define TAB '\t'
>+
>+#define CRSTR "\015"
>+#define LFSTR "\012"
>+#define CRLF "\015\012"     /* A CR LF equivalent string */

Might be annoying to extension authors to have these defined outside
of the NS_ namespace.  Do we really need these in the header?


> NS_strtok(const char *delims, char **str);
...
>+NS_COM_GLUE char NS_ToUpper(char aChar);

Hmm... lowercase and MixedCase.  I'd prefer some consistency in our style,
but I understand why you chose to name these methods like this.  Variants
on STDC methods are NS_lowercase and our methods are NS_MixedCase.  I guess
NSPR does the same, so whatever :-/

It might be worth it to comment on naming conventions for this file.


>Index: xpcom/glue/nsStringUtils.h

>+  nsDependentSubstring(const abstract_string_type& aStr,
>+                       PRUint32 aStartPos, PRUint32 aLength)
>+  {
>+    const PRUnichar* data;
>+    PRUint32 len = NS_StringGetData(aStr, &data);
>+
>+    if (aStartPos > len)
>+      aStartPos = len;
>+
>+    if (aStartPos + aLength > len)
>+      aLength = len - aStartPos;
>+
>+    NS_StringContainerInit2(*this, data + aStartPos, aLength,
>+                            NS_STRING_CONTAINER_INIT_DEPEND |
>+                            NS_STRING_CONTAINER_INIT_SUBSTRING);
>+  }

Maybe this shouldn't be inline.  There are probably some others like
that.  Feel free to punt such changes to a future bug.


>Index: xpcom/string/public/nsStringAPI.h

>+struct nsStringContainer_Size

Maybe nsStringContainer_size or _base instead?


>Index: xpcom/tests/TestMinStringAPI.cpp

>-#include "nsStringAPI.h"
>+#include "nsStringUtils.h"

I still wish people didn't have to change what they #include.  Maybe we could rename nsStringAPI.h to nsStringCore.h and nsStringUtils.h would become nsStringAPI.h.  What do you think?

Comment 8

[Benjamin Smedberg]

> Would be nice to use the same bracket style for all nsCRT methods
> that are defined inline.

Which style do you prefer? I was keeping them on one line if they didn't exceed 80 chars.

> >+#define FF '\f'
> 
> Might be annoying to extension authors to have these defined outside
> of the NS_ namespace.  Do we really need these in the header?

They are from nsCRT.h and are used all over the tree, unfortunately. I can probably move them back if you think it necessary (it's not like raw strings are that hard to read anyway).

> Maybe this shouldn't be inline.  There are probably some others like
> that.  Feel free to punt such changes to a future bug.

Yes let's... I need to add StringBeginsWith/StringEndsWith/StripWhitespace, so I'll do this at the same time.

> I still wish people didn't have to change what they #include.  Maybe we could
> rename nsStringAPI.h to nsStringCore.h and nsStringUtils.h would become
> nsStringAPI.h.  What do you think?

Ugh, I kinda like "API" actually being the API. Alternative:

C API: "nsXPCOMStrings.h"
C++ API: "nsStringAPI.h"

reasonable?

Comment 9

[Benjamin Smedberg]

Attachment: nsXPCOMStrings, rev. 2

Comment 10

[Darin Fisher]

Comment on attachment 220572 [details] [diff] [review]
nsXPCOMStrings, rev. 2

preserve cvs history for nsStringAPI.h/cpp?

indentation inconsistency in nsCRTGlue.cpp:NS_IsAscii

Comment 11

[Benjamin Smedberg]

Fixed on trunk

Comment 12

[Johnny Stenback (:jst)]

This change changed how nsCRT::strlen(PRUnichar *) works, it used to be null safe, now it's not. I'm not so sure that's a change we want to make this late, this is old code used all over the place and it already bit me once.

Comment 13

[Benjamin Smedberg]

There was a little discussion about that in bug 336737: I am happy enough to make nsCRT::strlen null-check before it forwards to NS_strlen, though I'd like to add a warning since I don't think it's a good programming metaphor in general.

Comment 14

[Johnny Stenback (:jst)]

Yeah, I agree that it's bad, it's an inconsistency between how the char* and PRUnichar* versions of nsCRT::strlen() works, but it's an inconsistency we've had for a *long* time and I know I've written code at times that relied on that, and I'm pretty sure I've seen other code that does as well. So unless we want to audit all uses of nsCRT::strlen(PRUnichar*) I think we should change it back, with a warning if we want to...

Comment 15

[Benjamin Smedberg]

nsCRT::strlen fixed (with a NS_ERROR) by the checkin for bug 337730