Replace calls to AppendASCII('*') with Append('*')

RESOLVED FIXED in mozilla23

Status

()

defect
RESOLVED FIXED
9 years ago
6 years ago

People

(Reporter: alfredkayser, Assigned: orva)

Tracking

(Blocks 1 bug, {memory-footprint, perf})

unspecified
mozilla23
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [mentor=Yoric][lang=c++])

Attachments

(1 attachment)

(Reporter)

Description

9 years ago
In mozilla/ xpcom/ io/ nsLocalFileWin.cpp  there are calls to AppendASCII which should be replaced by AppendLiteral, and for the case of one character with just Append('*') or Append('\\'); 
This prevent the need for a string, strlen, strcpy, etc.
(Reporter)

Updated

9 years ago
Assignee: nobody → alfredkayser
(Reporter)

Comment 1

6 years ago
http://mxr.mozilla.org/mozilla-central/search?string=AppendASCII%28%22&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=mozilla-central

The difference between AppendASCII("...") and AppendLiteral("...") is that the first also a does a "strlen" on the argument, whilst the second uses the compiler to pass the string length.
(Reporter)

Updated

6 years ago
Keywords: footprint, perf
I can mentor that.
Whiteboard: [mentor=Yoric][lang=c++]
(Assignee)

Comment 3

6 years ago
Posted patch patchSplinter Review
Simple patch, test cases left untouched. As this is my "low hanging fruit" to get familiar with Mozilla development process, all criticism is more than welcome.
Attachment #745201 - Flags: review?(dteller)
(Reporter)

Comment 4

6 years ago
This looks good to me!

Nit:
In nsLocalFileWin.cpp, I would replace:
738     if (filename.Last() == L'/' || filename.Last() == L'\\')
739         filename.AppendASCII("*");
740     else 
741         filename.AppendASCII("\\*");
742 
743     filename.ReplaceChar(L'/', L'\\');

with:
     filename.ReplaceChar(L'/', L'\\');
     if (filename.Last() != L'\\') filename.Append('\\');
     filename.Append('*');

But that is probably best for another bug.
Assignee: alfredkayser → iivari.aikas
Status: NEW → ASSIGNED
Comment on attachment 745201 [details] [diff] [review]
patch

Review of attachment 745201 [details] [diff] [review]:
-----------------------------------------------------------------

Looks good to me, too.
Passing to someone who has review rights on that directory.
Attachment #745201 - Flags: review?(dteller)
Attachment #745201 - Flags: review?(doug.turner)
Attachment #745201 - Flags: feedback+
Attachment #745201 - Flags: review?(doug.turner) → review+
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/865b9be24831
Status: ASSIGNED → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla23
(Reporter)

Updated

6 years ago
Blocks: 869826
You need to log in before you can comment on or make changes to this bug.