272277 - Change to Append/Assign/Equals-Literal in networking

Status: RESOLVED FIXED

(Core :: Networking, defect)

Description

[Mikael Parknert]

Change to Append/Assign/Equals-Literal in networking

Comment 1

[Mikael Parknert]

Attachment: proposed changes

Comment 2

[Darin Fisher]

replacing code like this:

  foo = NS_LITERAL_CSTRING("xyz") + str;

with

  foo.AssignLiteral("xyz");
  foo.Append(str);

is probably not worth it.  the advantage of using operator+ is that it causes
one assignment to be done.  the string buffer will be allocated to exactly the
right size the first time, and the null terminator will be written once.  the
operator+ code is pretty efficient, so i think unless you can provide a good
reason for this kind of change, i'd object to the change.

Comment 3

[Boris Zbarsky [:bzbarsky]]

I'm not going to be able to get to this review in the near future (at least one
week, possibly as long as five weeks).

Comment 4

[Mikael Parknert]

(In reply to comment #2)
'> is probably not worth it.  the advantage of using operator+ is that it causes
> one assignment to be done.  the string buffer will be allocated to exactly the
> right size the first time, and the null terminator will be written once.  the
> operator+ code is pretty efficient, so i think unless you can provide a good
> reason for this kind of change, i'd object to the change.

The main reason is remove bloat. This generates quite a lot of big code. Most
(but noll all) of the cases in the patch use a ns[C]AutoString and the
resulting strings will fit into the 64-sized buffer

Comment 5

[Darin Fisher]

Comment on attachment 167359 [details] [diff] [review]
proposed changes

>Index: netwerk/protocol/http/src/nsHttpHandler.cpp

>+        mOscpu.AssignLiteral("2.11");

Another option for this might be to make mOscpu be a
nsDependentCString.  Then you can call the "Rebind"
method here to avoid a heap allocation and memcpy :)

Same applies to some of the other strings that are
apparently assigned only literal values.


>Index: netwerk/protocol/http/src/nsHttpAuthCache.cpp

> GetAuthKey(const char *scheme, const char *host, PRInt32 port, nsCString &key)
> {
>-    key.Assign(nsDependentCString(scheme) +
>-               NS_LITERAL_CSTRING("://") +
>-               nsDependentCString(host) +
>-               NS_LITERAL_CSTRING(":"));
>+    key.Assign(scheme);
>+    key.AppendLiteral("://");
>+    key.Append(host);
>+    key.Append(':');
>     key.AppendInt(port);

OK, good point about codesize.	I agree.


>Index: netwerk/protocol/http/src/nsHttpConnectionInfo.cpp

>+    mHashKey.Append(nsPrintfCString(":%d", keyPort));

maybe change this to:

  .Append(':');
  .AppendInt(keyPort);

??


>Index: netwerk/protocol/about/src/nsAboutCache.cpp

>+    mBuffer.Truncate();
>     if (!mDeviceID.IsEmpty()) {
>+        mBuffer.AssignLiteral("</pre>\n");
>     }

perhaps moving the Truncate() call into an |else| clause would make
more sense?


sr=darin (with these nits fixed)

Comment 6

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

can you use the opportunity here and fix bug 134165?

Comment 7

[Mikael Parknert]

Attachment: updated with darin's comments

only mPlatform was made into an nsDependentCString

I also changed a local var into nsCAutoString (from nsCString)

as for bug 134165 - I'll attach a patch to that bug

Comment 8

[Mikael Parknert]

Comment on attachment 167575 [details] [diff] [review]
updated with darin's comments

I caught a problem with this patch

Comment 9

[Mikael Parknert]

Attachment: new patch

in the last patch I used an AssignLiteral instead of an AppendLiteral, oops

Comment 10

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

Comment on attachment 167769 [details] [diff] [review]
new patch

netwerk/protocol/http/src/nsHttpHandler.cpp
	 buf =	(char*)name.sysname;

hmm, wonder why that cast is needed...
anyway, not your code

netwerk/protocol/http/src/nsHttpDigestAuth.cpp
+    authString.AppendLiteral("auth");
     if (qop & QOP_AUTH_INT)
-      authString += "auth-int";
-    else
-      authString += "auth";
+      authString.AppendLiteral("-int");

hm... why this? seems like this code will be slightly less efficient in the
case where the if is hit... is there a gain?

netwerk/protocol/http/src/nsHttpConnectionInfo.cpp
-    mHashKey.Assign(NS_LITERAL_CSTRING("..") +
-		     nsDependentCString(keyHost) +
-		     nsPrintfCString(":%d", keyPort));
+    mHashKey.AssignLiteral("..");
+    mHashKey.Append(keyHost);
+    mHashKey.AppendInt(keyPort);

it seems like you lost a colon somewhere?

netwerk/base/src/nsURLHelper.cpp
+	     else if (name.EqualsLiteral(".") || name.IsEmpty()) {

hmm, might be better to test IsEmpty() first, since that check should be
cheaper...

Comment 11

[Darin Fisher]

Thanks for catching those errors biesi!

Comment 12

[Mikael Parknert]

Attachment: updated with biesi's comments

Biesi, thanks for the Append(':')  :-{

Comment 13

[Mikael Parknert]

Checked in 2004-12-04

Comment 14

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

+    mHashKey.Append(';');

not ':'?

Comment 15

[Mikael Parknert]

Thank you for finding that. Fixed
Next time i will read one extra time :-(