Remove nsCString::GetBuffer()

RESOLVED FIXED in mozilla0.8.1

Status

()

Core
String
P3
normal
RESOLVED FIXED
17 years ago
17 years ago

People

(Reporter: Peter ``jag'' Annema, Assigned: Peter ``jag'' Annema)

Tracking

Trunk
mozilla0.8.1
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(8 attachments)

(Assignee)

Description

17 years ago
GetBuffer() has been depricated in favor of get() to get more unity in code.

This bug will be for tracking changing of all nsCString::GetBuffer() uses to
nsCString::get() so GetBuffer can be removed.
(Assignee)

Comment 1

17 years ago
Created attachment 21491 [details] [diff] [review]
[patch] step 1a, remove most GetBuffer clients (still some left)
(Assignee)

Comment 2

17 years ago
Created attachment 21493 [details] [diff] [review]
[patch] step2: fix nsString.h and nsString2.cpp

Comment 3

17 years ago
do you seriously expect anyone to review this ;-?

notes to self:
write("T",1), += " ",
non PR ints..
comparisons w/ nsnull, null, 0
.Length() > 0
initializers should be in the order of class definition

General questions:
title.AssignWithConversion("Error");        // localization
rv = 
picker->SetDefaultString(NS_ConvertASCIItoUCS2(suggested.get()).GetUnicode() );
rv =a => rv = a

I think there was only one rv =a, please fix that. Cleaning up the other 
nonsense is for someone else.

Updated

17 years ago
Keywords: approval, patch, review
(Assignee)

Comment 4

17 years ago
Well, I'm hoping someone will, seeing how I've kept this patch mostly to fixing
|GetBuffer()| uses, and do explicit |get()| where needed (though not everywhere
yet). If I'd add all that other fixing up I'm sure this will just gather dust.
Here are comments on the first part of the first patch, up to and including
nsAbCard.cpp:

nsLocationImpl.cpp:
  filed bug 64041 on other issues

nsLocalService.cpp:
  should use langcstr.ToNewCString()  (2 times)
  should untabify?

nsDocumentEncoder.cpp:
  This code (around the first change) is rather evil.  Maybe you should fix it,
  or at least file a bug on its owner?
(Assignee)

Comment 6

17 years ago
Created attachment 21549 [details] [diff] [review]
[patch] step1a-update1: fixes for comments timeless & dbaron
(Assignee)

Comment 7

17 years ago
| rv =a => rv = a

| nsLocalService.cpp:
|   should use langcstr.ToNewCString()  (2 times)
|   should untabify?

Those are done, and some more strdup -> ToNewCString() . strlen -> Length()
changes.

| nsDocumentEncoder.cpp:
|   This code (around the first change) is rather evil.  Maybe you should
|   fix it, or at least file a bug on its owner?

I guess I'll file a bug...
(Assignee)

Comment 8

17 years ago
Created attachment 21662 [details] [diff] [review]
[patch] step1b: More GetBuffer -> get
r=dbaron on attachment 21491 [details] [diff] [review] up to and including nsImapIncomingServer.cpp, with
the following comments:

nsAddressBook.cpp:
 * should file bug on AddressBookParser::AddLdifColToDatabase to switch
   colType from nsCAutoString to nsLiteralCString and use new string
   tools in nsReadableUtils.h

 * Perhaps it would be better to rename the valueSlot parameter to
   column rather than changing all the uses.

nsMsgThread.cpp:
 * Perhaps it would be better to reorder the initializers rather than
   the members themselves?  It seems the members are more likely
   to be in the "intended" order.
(Assignee)

Comment 10

17 years ago
It turns out someone else already checked in the change to nsMsgThread.cpp, but
in general I think you're right that it's better to change the initializer order
than the member order.

Attaching a patch for the issues in nsAddressBook.cpp, will hopefully remember
to file that new bug :-) (Perhaps turn it into a general clean-up one? Remove
some CIDs, replace WITH_SERVICE macros, etc.)
(Assignee)

Comment 11

17 years ago
Created attachment 23711 [details] [diff] [review]
[patch] step1a-nsAddressBook.cpp

Comment 12

17 years ago
I'd like to have all the |.get| changes in for mozilla0.9.
Status: NEW → ASSIGNED
Target Milestone: --- → mozilla0.9
(Assignee)

Comment 13

17 years ago
Great :-) How best can we get that done? (Other than r=/a= grunt work).

Updated

17 years ago
Priority: -- → P3

Updated

17 years ago
Depends on: 70075

Updated

17 years ago
Depends on: 70090

Updated

17 years ago
No longer depends on: 70090

Updated

17 years ago
Blocks: 70090
(Assignee)

Comment 14

17 years ago
Okay, started from scratch with a new patch and way less noise.

Taking this bug from scc so I have it on my radar.
Assignee: scc → disttsc
Status: ASSIGNED → NEW
(Assignee)

Comment 15

17 years ago
Created attachment 26178 [details] [diff] [review]
[patch] This should remove all nsCString::GetBuffer()

Comment 16

17 years ago
builds on Mac :-)
(Assignee)

Comment 17

17 years ago
Created attachment 26508 [details] [diff] [review]
[patch] update to nsLocaleService.cpp after recent checkin to it. Decided to clean up :-)

Comment 18

17 years ago
26508 r=timeless
26178 r=timeless w/ few requests
NS_CONST_CAST(char*,var.get()),(char*)var.get() => var.get()
new nsCString(var.get()) => var.ToNewCString()
afaik .Append*(var.get) drop .get() [also for .Equals?]
(Assignee)

Comment 19

17 years ago
> NS_CONST_CAST(char*,var.get()),(char*)var.get() => var.get()

Specifically for |->SetSpec| on nsIURI. Done.

timeless: some of those (char*) / NS_CONST_CAST(char*, ...) are needed for
ugliness like nsUnescape.

> new nsCString(var.get()) => var.ToNewCString()

Actually |.ToNewString()|, but it just does |return new nsCString(var);|, so
I'll just drop the |.get()|. Done.

> afaik .Append*(var.get) drop .get() [also for .Equals?]

Done.

I've also removed |.get()| in |new nsCStringKey(var.get());|

I'm building now with these changes, I'll attach the new patch when that
compiles fine.
(Assignee)

Comment 20

17 years ago
Created attachment 26612 [details] [diff] [review]
[patch] incorporated timeless' comments, synch'ing
(Assignee)

Comment 21

17 years ago
Patch mostly checked in. nsString.h change this weekend.

Updated

17 years ago
Target Milestone: mozilla0.9 → mozilla0.8.1
(Assignee)

Comment 22

17 years ago
*grmbl*

Thanks syd. Take all the fun out of it, why don't you ;-p

Marking this fixed.
Status: NEW → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.