137489 - hierarchyDelimiter should consistently be a char

Status: RESOLVED FIXED

(MailNews Core :: Networking: IMAP, defect)

Description

[timeless]

F:\build\mozilla\mailnews\imap\src\nsImapIncomingServer.cpp(1310) : warning 
C4244: 'argument' : conversion from 'unsigned short' to 'char', possible loss 
of data

1122 NS_IMETHODIMP nsImapIncomingServer::PossibleImapMailbox(const char 
*folderPath, PRUnichar hierarchyDelimiter, PRInt32 boxFlags, PRBool 
*aNewFolder)
1310 dupFolderPath.ReplaceChar('/', hierarchyDelimiter);

Is there a real need for hierarchyDelimiter to be a PRUnichar?

bienvenu: no, there's not - I think whoever ported that code thought we would 
use prunichar's everywhere and we never fixed it.

Comment 1

[David :Bienvenu]

*** Bug 160650 has been marked as a duplicate of this bug. ***

Comment 2

[Karen Huang]

Adding Henry's comments from bug 160650:
--------------------------------------------------------------------------------
nsIMsgImapMailFolder uses 'PRUnichar' delimiter, nsImapUrl uses 'char'
delimiter, nsIImapMailFolderSink uses 'char' delimiter, nsISubscribableServer
uses 'char' delimiter...

I think we should unify the data type because we often set the value from one to
another. I suggest to use 'char' to reduce the memory usage.
----------------------------------------------------------------------------------

Comment 3

[Henry Jia]

Attachment: patch for review

Comment 4

[Henry Jia]

Taking.

Add mscott to CC list.

Comment 5

[Henry Jia]

Add navin & Cavin to the CC list.

Pls r=. Thx.

Comment 6

[Navin Gupta]

Comment on attachment 93990 [details] [diff] [review]
patch for review

r=naving

Comment 7

[Henry Jia]

Comment on attachment 93990 [details] [diff] [review]
patch for review

David, please take a look at. Thx.

Comment 8

[Matthias Versen [:Matti]]

bienvenu: Do you forget this review ?

Comment 9

[David :Bienvenu]

this patch won't reduce memory useage or really have any effect at all, but we
can look at getting it in after 1.4 is done.

Comment 10

[Henry Jia]

But it can simplify the usage of hierarchyDelimiter. :-)

Comment 11

[Mike Kaply [:mkaply]]

two year old sr request. Anyone want to do anything?

Comment 12

[Alec Flett]

Comment on attachment 93990 [details] [diff] [review]
patch for review

first thing I'll do is request a new patch. I'm sure this one is log since
stale.
It looks fine though, it ought to be a quick review on the new patch.

Comment 13

[Wayne Mery (:wsmwk)]

Alec, do you want to sign up as assignee?

Comment 14

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Comment on attachment 93990 [details] [diff] [review]
patch for review

Patch has bitrotted.

$ patch -p0 --dry-run < ~/Desktop/tbTestPatches/bz137489.diff 
(Stripping trailing CRs from patch.)
patching file nsIImapServerSink.idl
Hunk #1 FAILED at 44.
1 out of 1 hunk FAILED -- saving rejects to file nsIImapServerSink.idl.rej
(Stripping trailing CRs from patch.)
can't find file to patch at input line 25
Perhaps you used the wrong -p or --strip option?
The text leading up to this was:
--------------------------
|
|Index: nsImapIncomingServer.cpp
|===================================================================
|RCS file: /cvsroot/mozilla/mailnews/imap/src/nsImapIncomingServer.cpp,v
|retrieving revision 1.264
|diff -u -r1.264 nsImapIncomingServer.cpp
|--- nsImapIncomingServer.cpp   23 Jul 2002 02:27:19 -0000      1.264
|+++ nsImapIncomingServer.cpp   5 Aug 2002 07:51:23 -0000
--------------------------
File to patch:

Comment 15

[Magnus Melin [:mkmelin]]

Attachment: proposed fix

Comment 16

[David :Bienvenu]

Comment on attachment 379209 [details] [diff] [review]
proposed fix

+      if (hierarchyDelimiter != kOnlineHierarchySeparatorUnknown &&
+          onlineSubDirDelimiter != hierarchyDelimiter)
+          m_runningUrl->SetOnlineSubDirSeparator(hierarchyDelimiter);

last line shouldn't be indented so much.

While you're doing this (thx!, btw), can you replace hierarchySeparator w/ hierarchyDelimiter - the IMAP RFC calls it a hierarchyDelimiter and we should be consistent.

r/sr=me with those nits addressed.

Comment 17

[Magnus Melin [:mkmelin]]

Attachment: proposed fix, v2 (as checked in)

changeset:   2775:e14d90899db9
http://hg.mozilla.org/comm-central/rev/e14d90899db9

Comment 18

[Magnus Melin [:mkmelin]]

->FIXED