Last Comment Bug 544621 - Use or remove '#ifdef HAVE_PORT' code
: Use or remove '#ifdef HAVE_PORT' code
Status: RESOLVED FIXED
:
Product: MailNews Core
Classification: Components
Component: Backend (show other bugs)
: Trunk
: All All
: -- trivial (vote)
: Thunderbird 22.0
Assigned To: :Cykesiopka
:
Mentors:
http://mxr.mozilla.org/comm-central/s...
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2010-02-05 18:39 PST by Serge Gautherie (:sgautherie)
Modified: 2013-03-28 06:16 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---


Attachments
Proposed Patch v1 (6.66 KB, patch)
2013-03-20 16:01 PDT, :Cykesiopka
standard8: review-
Details | Diff | Review
Patch v2 (13.11 KB, patch)
2013-03-27 16:16 PDT, :Cykesiopka
standard8: review+
Details | Diff | Review

Description Serge Gautherie (:sgautherie) 2010-02-05 18:39:58 PST
(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
}
Comment 1 Serge Gautherie (:sgautherie) 2010-02-05 18:44:16 PST
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
Comment 2 :Cykesiopka 2013-03-20 16:01:40 PDT
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...
Comment 3 Mark Banner (:standard8) 2013-03-27 04:07:35 PDT
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
Comment 4 Mark Banner (:standard8) 2013-03-27 04:08:08 PDT
As you're working on this, assigning it to you for tracking purposes.
Comment 5 :Cykesiopka 2013-03-27 15:14:30 PDT
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...
Comment 6 Mark Banner (:standard8) 2013-03-27 15:20:32 PDT
(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.
Comment 7 :Cykesiopka 2013-03-27 16:15:05 PDT
Thanks for the clarification!
Comment 8 :Cykesiopka 2013-03-27 16:16:15 PDT
Created attachment 730403 [details] [diff] [review]
Patch v2

+Dead code removal
Comment 9 Mark Banner (:standard8) 2013-03-28 03:47:26 PDT
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 :-)
Comment 10 Ryan VanderMeulen [:RyanVM] 2013-03-28 06:16:53 PDT
https://hg.mozilla.org/comm-central/rev/5b9c506ce030

Note You need to log in before you can comment on or make changes to this bug.