78032 - ++iter optimizes better than iter++, fix callers (in tight loops)

Status: RESOLVED WORKSFORME

(Core :: XPCOM, defect, P3)

Description

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

As jag pointed out in bug 65431 and I tested, the statement |++iter| optimizes
better, at least on gcc 2.96-76 with -O2, than |iter++|.  We should probably
convert tight loops that use string iterators (such as the ones in
nsScanner::ReadUntil) to use the prefix form, or we should figure out how to
make the compiler optimize the postfix as well as the prefix.

See attachment 32389 [details], "differences between prefix and postfix ++ with
gcc-2.96-76 -O2".

Comment 1

[Scott Collins]

Of _course_ pre-increment is better!  That's why I'm always pushing people to
develop the habit of preferring pre-increment over post-increment except when
demanded by idiom.  This is a well known rule of C++.  See "Effective C++" for
details.  Post-increment typically requires a temporary (when implementing
standard behavior for user-defined types) which the C++ compiler can almost
never optimize away if you really are in a situation where post-increment is needed.

There are two reasons to prefer pre-increment.  First: ``say what you mean''. 
What you mean is ``increment this variable''.  Not ``tell me the old value, then
increment this variable''.  Second: the efficiency concern with respect to
user-defined types, particular complex iterators, e.g., for mozilla strings or
for containers from the standard C++ library.  Consider other differences as
well, the result of post-increment must be a |const| rvalue, while the result of
pre-increment is a reference.

  ++ ++i // is legal for any built-int type, and should be for a user-defined
  i++ ++ // turns out _not_ to be legal for built-in types

post-increment should be considered harmful except where demanded by idiom

Comment 2

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

Hmm.  Maybe we should start writing in ++C...

Comment 3

[Peter "jag" Annema]

So should this be resolved invalid? Too bad we can't resolve it Documentation...

Comment 4

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

No, we should fix callers.  That was what this bug was about...

I was thinking we might even be able to come up with some debug hack that
asserted when iter++ was used and the value dropped (such as having the iter++
return a struct wrapping the new iterator (via private inheritance?) with an
operator to cast it to the iterator, and an assertion in the destructor of the
struct if the cast operator was never called).

Comment 5

[Peter "jag" Annema]

You're right. I didn't re-read the first comment and I associated this bug with
just the "we should figure out how to make the compiler optimize the postfix as
well as the prefix" part.

Changing summary.

Comment 6

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

Attachment: patch to break postfix ++ and -- so the compiler can find uses (not for checkin)

Comment 7

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

Attachment: patch to convert all postfix ++/-- *statements* to prefix (and a some nearby integers for consistency)

Comment 8

[Peter Van der Beken [:peterv]]

r=peterv for the change to nsStyleLinkElement.cpp.

Comment 9

[Scott Collins]

sr=scc for the stuff in the strings directory

Comment 10

[harishd]

r=harishd

Comment 11

[Dan Mosedale (:dmosedale, :dmose)]

r=[e-mail] for the LDAP changes

Comment 12

[David Hyatt]

sr=hyatt

Comment 13

[Asa Dotzler [:asa]]

a= [e-mail] for checkin to the trunk.
(on behalf of drivers)

Comment 14

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

Patch checked in 2001-06-19 15:38 PDT.

Moving to future so I can go through this exercise again at some point in the
future.

Comment 15

[Serge Gautherie (:sgautherie)]

Comment on attachment 38762 [details] [diff] [review]
patch to break postfix ++ and -- so the compiler can find uses (not for checkin)

Even if this patch is for "debug use only" and not "trunk checkin",
In <nsScanner.cpp>, Shoudn't |mCountRemaining--;| be converted to
|--mCountRemaining;| ?
For obvious reason ;->

Comment 16

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

Should no longer be a problem with new stirng code.