Use or remove '#ifdef HAVE_PORT' code

RESOLVED FIXED in Thunderbird 22.0

Status

MailNews Core
Backend
--
trivial
RESOLVED FIXED
8 years ago
4 years ago

People

(Reporter: sgautherie, Assigned: Cykesiopka)

Tracking

Trunk
Thunderbird 22.0

Firefox Tracking Flags

(Not tracked)

Details

(URL)

Attachments

(1 attachment, 1 obsolete attachment)

13.11 KB, patch
standard8
: review+
Details | Diff | Splinter Review
(Reporter)

Description

8 years ago
(Noticed while working on bug 530414.)

{
/mailnews/local/src/nsLocalMailFolder.cpp
    * line 1390 -- #ifdef HAVE_PORT
/mailnews/imap/src/nsImapProtocol.cpp
    * line 2835 -- #ifdef HAVE_PORT
}
(Reporter)

Comment 1

8 years ago
Fwiw,
some of these were added in r1.1
and removed in
http://bonsai.mozilla.org/cvslog.cgi?file=mozilla/mailnews/local/src/nsLocalMailFolder.cpp&mark=1.428#1.429
(Assignee)

Comment 2

4 years ago
Created attachment 727402 [details] [diff] [review]
Proposed Patch v1

I can't even compile TB if I #define HAVE_PORT in either .cpp file or if I remove the ifdefs... So I guess removing this code is the way to go...
Attachment #727402 - Flags: review?(mozilla)
Comment on attachment 727402 [details] [diff] [review]
Proposed Patch v1

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

Thanks for doing the patch. I think we could go a little bit further however, looking at the (get)requiresCleanup function:

http://mxr.mozilla.org/comm-central/search?string=RequiresCleanup&find=&findi=&filter=^[^\0]*%24&hitlimit=&tree=comm-central

it isn't actually called anywhere, additionally, all the functions in the other files either don't return anything, return an error, or return false. So I think we can drop all the instances of requiresCleanup that you can see in that link - clearRequiresCleanup.

Could you update the patch to remove the redundant functions?

Note that because you'll also remove functions from nsIMsgFolder.idl, you'll also need to change the uuid of nsIMsgFolder (near the top of the file), information on how to get a new uuid is available here: https://developer.mozilla.org/en-US/docs/Generating_GUIDs
Attachment #727402 - Flags: review?(mozilla) → review-
As you're working on this, assigning it to you for tracking purposes.
Assignee: nobody → cykesiopka
(Assignee)

Comment 5

4 years ago
For clarification, do you want me to remove all of the following?
  - boolean requiresCleanup & clearRequiresCleanup() in nsIMsgFolder.idl
  - All the GetRequiresCleanup() implementations
  - All the ClearRequiresCleanup() implementations

I'm unsure of the ClearRequiresCleanup() stuff in particular, sorry...
Flags: needinfo?(mbanner)
(In reply to Cykesiopka from comment #5)
> For clarification, do you want me to remove all of the following?
>   - boolean requiresCleanup & clearRequiresCleanup() in nsIMsgFolder.idl
>   - All the GetRequiresCleanup() implementations
>   - All the ClearRequiresCleanup() implementations

Yes, all of the above.

> I'm unsure of the ClearRequiresCleanup() stuff in particular, sorry...

ClearRequiresCleanup isn't called from anywhere and only is defined in one place, so it is safe to remove.

A quick summary of what is happening here: the functions are defined as interfaces in nsIMsgFolder.idl, there's then various classes that inherit from nsIMsgFolder and implement those functions. So if you remove them from nsIMsgFolder.idl (which is safe to do as I previously noted), you'll need to remove the implementations as well.

You'll also note that once removed from nsIMsgFolder, if you haven't removed the implementations you will get compile issues.

Feel free to send me patches or ask me other questions direct if you need more help here.
Flags: needinfo?(mbanner)
(Assignee)

Comment 7

4 years ago
Thanks for the clarification!
(Assignee)

Comment 8

4 years ago
Created attachment 730403 [details] [diff] [review]
Patch v2

+Dead code removal
Attachment #727402 - Attachment is obsolete: true
Attachment #730403 - Flags: review?(mbanner)
Comment on attachment 730403 [details] [diff] [review]
Patch v2

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

That's excellent. Thanks for the patch, a nice bit of cleanup :-)
Attachment #730403 - Flags: review?(mbanner) → review+
Keywords: checkin-needed
https://hg.mozilla.org/comm-central/rev/5b9c506ce030
Status: NEW → RESOLVED
Last Resolved: 4 years ago
Keywords: checkin-needed
Resolution: --- → FIXED
Target Milestone: --- → Thunderbird 22.0
You need to log in before you can comment on or make changes to this bug.