Closed Bug 544621 Opened 11 years ago Closed 7 years ago

Use or remove '#ifdef HAVE_PORT' code

Categories

(MailNews Core :: Backend, defect)

defect
Not set
trivial

Tracking

(Not tracked)

RESOLVED FIXED
Thunderbird 22.0

People

(Reporter: sgautherie, Assigned: Cykesiopka)

References

()

Details

Attachments

(1 file, 1 obsolete file)

(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
}
Attached patch Proposed Patch v1 (obsolete) — Splinter Review
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
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)
Thanks for the clarification!
Attached patch Patch v2Splinter Review
+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+
https://hg.mozilla.org/comm-central/rev/5b9c506ce030
Status: NEW → RESOLVED
Closed: 7 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.