78993 - Properly "inline" nsLDAPServiceEntry

Status: RESOLVED INACTIVE

(Directory :: LDAP XPCOM SDK, defect, P2)

Description

[Leif Hedstrom]

From Brendans SR= of 70422:

The other followup to-do: really inline these getters (and maybe make them XPCOM
style, if we can't have already_AddRefed; it's incongruous to see an XPCOM-style
out param passed using getter_AddRefs for operation->GetConnection, while the
entry->GetConnection uses the return value).

/be

Comment 1

[Leif Hedstrom]

I'm also adding these methods to the |nsLDAPServiceEntry|:

|HasMessage()|
|HasConnection()|


and to the |nsILDAPService| interface we're adding

|HasConnection()|


I have a patch for this already, which I'll post here once I've tested it on Mac
and Win32.

Comment 2

[Leif Hedstrom]

Attachment: First patch

Comment 3

[Leif Hedstrom]

I've moved all the code for the |nsLDAPServiceEntry| to the |.h| file, and added
the methods mentioned in the bug comment. I still need to test this on Macintoy
(tomorrow).

Dmose, can you review these changes please?

Thanks,

-- Leif

Comment 4

[yulian chang]

reassign to Olga as QA contact

Comment 5

[Dan Mosedale (:dmosedale, :dmose)]

Review comments:

* drop hasConnection, since, as we discussed yesterday, the semantics are not
  what we really want.

* is it really necessary to move all the inline function code into the .h file?
 Would just adding inline to the actual method definitions be sufficient?

* any particular reason not inline nsLDAPServiceEntry::Init?

Comment 6

[Brendan Eich [:brendan]]

I dunno about inline in the .cpp files, and I still say these data members
should be exposed to their friends (no snickering).  Care to find out what does
and does not inline?  Anyway, fewer source lines and inlines in the class sound
better to me.  If anyone ever uses one elsewhere, you won't be calling an out of
line copy.

/be

Comment 7

[Dan Mosedale (:dmosedale, :dmose)]

brendan: 

As far as looking at what gets inlined and what doesn't, that seems like
premature optimization to me.  This is exceedingly unlikely to be any sort of
bottleneck, I think.

I'm a little unclear on what exactly you're asking for in the second to last
sentence.  Cutting down on the necessary (inlined) function calls by grovelling
in nsLDAPServiceEntry's members directly?  I'll buy that.  

I assume the last sentence is referring to the fact that |inline| is just a hint
to the compiler, and it might not always happen?

Comment 8

[Brendan Eich [:brendan]]

dmose: didn't you read my anti-"patternism" postscript in leif's cvs access
bug?  All these little get and set functions, with their non-XPCOM special
patterns, just obfuscate the code and make it hard to "grep" for thread-unsafe
or deadlock prone call-outs.

Inlining one-line getters used in only one file, if you don't go with exposed
data members (via friend or public) is a premature deoptimization that will
never go away "later", and that compounds linearly in code space and cycles.  Do
this 1000x and then come back asking where performance went, compared to the C
version from four years ago.  No, wait, don't -- that would suck!

/be

Comment 9

[Leif Hedstrom]

I'm tired, and confused. What exactly are we saying now? Should I not "inline"
these methods in the include file now? Change them to XPCOM style parameter
passing instead?

I thought I understood what was asked for, but I guess not.

Comment 10

[Brendan Eich [:brendan]]

leif: I still think moving those functions wholesale into the class decl in the
.h file is the best near-term change.  I also don't see the point in hiding data
from one other file that is friends with it.  But I'm not the boss of you!  Do
what you think is best.

/be

Comment 11

[Leif Hedstrom]

Ok. So besides removing that new |HasConnection()|, is that patch what you
expect? I didn't inline the |Init()| function, I couldn't think of a reason why
that would be useful.

Thanks,

-- leif

Comment 12

[Brendan Eich [:brendan]]

Yeah, don't inline Init.  Also, too much vertical space here:

+    nsLDAPServiceEntry()
+        : mLeases(0),
+          mDelete(PR_FALSE),
+          mRebinding(PR_FALSE)
+
+    {
+        mTimestamp = LL_Zero();
+    }

And you don't need the inline keyword if the method is defined (not just decl'd)
in the class.  Some people prefer one-liner get and set methods, and I do too --
but don't scrunch too much onto one line, ok?

Sorry I didn't review sooner.  At this point, fixing the above nits, sight
unseen by me gets you an sr=, so don't sweat it.

/be

Comment 13

[Leif Hedstrom]

Dmose

Comment 14

[scottputterman]

Moving out of 0.9.7.  Feel free to move them past 0.9.8.

Comment 15

[Dan Mosedale (:dmosedale, :dmose)]

Mass change: pushing out 0.9.8 bugs, as I'm concentrating feature work now.

Comment 16

[Dan Mosedale (:dmosedale, :dmose)]

Assigning bugs that I'm not actively working on back to nobody; use SearchForThis as a search term if you want to delete all related bugmail at once.

Comment 17

[Serge Gautherie (:sgautherie)]

Filter on "Nobody_NScomTLD_20080620"

Comment 18

[Ben Campbell]

Moot point.
nsLDAPService wasn't being used, and was mostly culled in Bug 1672902 (https://hg.mozilla.org/comm-central/rev/75d7bbd322cc).