Bug 101859 – Use prefix form of increment / decrement operators in WTF String classes when possible

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Chris Dumez]

The WTF String classes implementation uses the postfix form of increment / decrement operators in several places where it is not needed. We should prefer using the prefix increment / decrement operators when possible as it is according to WebKit coding style and inherently more efficient.

[Chris Dumez]

Created attachment 173500 [details]
Patch

[Andreas Kling]

Comment on attachment 173500 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173500&action=review

> Source/WTF/ChangeLog:10
> +        Use prefix form of increment / decrement operators whenever possible in
> +        WTF String classes as this is according to WebKit coding style and
> +        inherently more efficient.

Why? Postfix increment/decrement does not generate any additional code for primitive types.

[Chris Dumez]

(In reply to comment #2)
> (From update of attachment 173500 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173500&action=review
> 
> > Source/WTF/ChangeLog:10
> > +        Use prefix form of increment / decrement operators whenever possible in
> > +        WTF String classes as this is according to WebKit coding style and
> > +        inherently more efficient.
> 
> Why? Postfix increment/decrement does not generate any additional code for primitive types.

Right, they are all primitive types so it would not affect performance. I'll remove the efficiency comment from the Changelog then.

I still think it is good practice to use prefix operators whenever possible and WebKit seemed to follow this rule in most places I have seen.

[Chris Dumez]

Created attachment 173509 [details]
Patch

Remove the comment about efficiency from the Changelog since all changes relate to primitive types and it does not affect performance in this case as Kling rightfully mentioned.

Is it right that we have such coding convention in WebKit though?
This patch was basically a follow up to https://bugs.webkit.org/show_bug.cgi?id=101257#c22 : 
"Typically ++i in WebKit style."

[Anders Carlsson]

In general, we try not to do pure coding style cleanup patches. Instead, we make sure that any new code follows the style guidelines, and when changing a line of code it's fine to update it to follow the guidelines.

[Benjamin Poulain]

Comment on attachment 173509 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=173509&action=review

Consistency is good and we are already in a period of coding style patches :)

> Source/WTF/wtf/text/StringImpl.cpp:1863
> -    terminatedString->m_length--;
> +    --terminatedString->m_length;

I'd argue this one becomes harder to read with the change.
I would prefer with parenthesis or with the old form.

[Chris Dumez]

Created attachment 173692 [details]
Patch for landing

Added parenthesis for "--(terminatedString->m_length);" to increase readability.

[Anders Carlsson]

(In reply to comment #6)
> (From update of attachment 173509 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=173509&action=review
> 
> Consistency is good and we are already in a period of coding style patches :)

We are? That's unfortunate since they pollute svn blame for no good reason.

[Benjamin Poulain]

Comment on attachment 173692 [details]
Patch for landing

Ok, let's go for it.
Rationale:
This code cleaning is nice. It unify the style with what is ultimately very few changes.
Blame is useless here anyway since most of that has be written before WTF was moved.

[Anders Carlsson]

(In reply to comment #9)
> Blame is useless here anyway since most of that has be written before WTF was moved.

Moving WTF out into a separate module did _not_ break svn blame.

[Benjamin Poulain]

(In reply to comment #10))
> > Blame is useless here anyway since most of that has be written before WTF was moved.
> 
> Moving WTF out into a separate module did _not_ break svn blame.

Oh, I did not know that. On GIT it certainly fucks things up, but it is very easy to live with it.

[WebKit Review Bot]

Comment on attachment 173692 [details]
Patch for landing

Clearing flags on attachment: 173692

Committed r134514: <http://trac.webkit.org/changeset/134514>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.