173894 - cleanup QIs in nsComposerCommands.cpp

Status: RESOLVED FIXED

(SeaMonkey :: Composer, defect)

Description

[Kathleen :Brade]

As part of another bug, a comment was made about making the QI's in this file
more efficient.  That work was not done in that bug so this bug will cover that
issue.

Comment 1

[Kathleen :Brade]

Attachment: clean up QI calls

Comment 2

[Axel Hecht]

Comment on attachment 102549 [details] [diff] [review]
clean up QI calls

r=[e-mail]
someone should clean up the the inconsistent use of
if (!foo) return NS_ERROR
some day. I prefer two lines.

Comment 3

[Peter Van der Beken [:peterv]]

Comment on attachment 102549 [details] [diff] [review]
clean up QI calls

sr=peterv. Cleaning up the return style indeed would be nice some day.

Comment 4

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Is there really any difference in efficiency between these two forms?  Don't
optimizers optimize away the difference?  (Do you see any difference in the
generated code in a non-DEBUG build?)  I vaguely remember an issue like this
where there was no difference on most compilers, and on the one where there was
a difference, the theoretically slower version was faster.  bbaetz might remember.

Comment 5

[Bradley Baetz (:bbaetz)]

There is no difference. In fact, on gcc-2.96, for non optimised builds only, the
= form is in fact 1 byte shorter (No, I don't know why). It was a while ago when
I tested, and I don't know if that still holds true for gcc-3.2, but since any
optmisiation level makes it moot, I don't think that we should care.

See http://scottcollins.net/programming/cpp/initialization.html

Comment 6

[Kathleen :Brade]

I think it was alecf who requested this form be used
AlecF--can you please post your comments here?

Comment 7

[Alec Flett]

Comment on attachment 102549 [details] [diff] [review]
clean up QI calls

no.. it was not me and in fact this is completely unnecessary. There was a time
that we believed that this saved a few instructions to call the default
constructor for nsCOMPtr<T> before calling the = operator..

then it turned out that most compilers are pretty darn smart and that the
compled code was more or less the same (and I think on some compilers was even
identical)

I personally find this form less readable, and would be against making changes
in a module I owned. I don't recommend it.

Comment 8

[Kathleen :Brade]

Comment on attachment 102549 [details] [diff] [review]
clean up QI calls

hmmmm... sorry alecf!  I don't recall which super-reviewer wanted it the other
way (it wasn't one of my bugs that this issue came up on).

I will redo the patch to be consistent the other way.

Thanks!

Comment 9

[Kathleen :Brade]

I think it was Darin who requested the other format for the do_QueryInterface

Comment 10

[Darin Fisher]

hrm... well, ok, i stand corrected then.  a year or so ago i asked about this
because a number of people (i forget who) told me to not use operator= for
nsCOMPtr initialization.  i wasn't convinced until i asked scc about it.  he
said he had done some simple tests to confirm that more code is generated and
that that would obviously require more of the optimizer to eliminate the
unnecessary code.  he said that most compilers get this right, but that it would
probably not be a bad idea to use the "less code" form.  i guess i missed the
thread about the fact that we are happy with the optimized code generated on all
platforms :-/

Comment 11

[Alec Flett]

yeah, there definitely was a time when we tried to avoid operator= at all costs,
but I think it was down to a difference of something like one instruction. "We"
decided (I think really just scc decided and word spread) it didn't really
matter THAT much. I think basically we got less strict about it when we realized
how minor the impact actually was.

Comment 12

[Kathleen :Brade]

at a glance, it now looks like we consistently use = in the file so resolving
this as fixed