Allow constexpr, and use it in a few places.

Allow constexpr, and use it in a few places.

BUG=none
TEST=none

Committed: https://crrev.com/ce246fab1effdfa0cc52e3954b86690f389a9e5d
Cr-Commit-Position: refs/heads/master@{#387355}

[Peter Kasting, 2016-04-13 20:59:48]

pkasting@chromium.org changed reviewers:
+ danakj@chromium.org, estade@chromium.org

[Peter Kasting, 2016-04-13 20:59:48]

estade: theme_properties.cc OWNERS
danakj: ui/gfx OWNERS and formal C++11 owners approval

[Nico, 2016-04-13 21:06:11]

thakis@chromium.org changed reviewers:
+ thakis@chromium.org

[Nico, 2016-04-13 21:06:12]

https://codereview.chromium.org/1885183002/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_properties.cc (right):

https://codereview.chromium.org/1885183002/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_properties.cc:103: constexpr SkColor
kDefaultColorNTPBackground = SK_ColorWHITE;
Does this help any? const variables of integral type with initializer already
are constexpr, so this change doesn't really do anything, does it?

[Peter Kasting, 2016-04-13 21:25:09]

https://codereview.chromium.org/1885183002/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_properties.cc (right):

https://codereview.chromium.org/1885183002/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_properties.cc:103: constexpr SkColor
kDefaultColorNTPBackground = SK_ColorWHITE;
On 2016/04/13 21:06:12, Nico wrote:
> Does this help any? const variables of integral type with initializer already
> are constexpr, so this change doesn't really do anything, does it?

On this specific line, no, but it does for some of the other lines here (e.g.
the HSL stuff, allowing kDefaultTintButtonsIncognito to reference
kDefaultTintButtons without adding a static initializer) and I read both the
Google styleguide and Effective Modern C++ as saying to use constexpr
consistently when declaring compile-time constants, so it seemed to make sense
to me to change all such expressions atop this file at the same time.

[danakj, 2016-04-13 22:08:21]

LGTM

[Nico, 2016-04-13 22:49:15]

https://codereview.chromium.org/1885183002/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_properties.cc (right):

https://codereview.chromium.org/1885183002/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_properties.cc:103: constexpr SkColor
kDefaultColorNTPBackground = SK_ColorWHITE;
On 2016/04/13 21:25:09, Peter Kasting wrote:
> On 2016/04/13 21:06:12, Nico wrote:
> > Does this help any? const variables of integral type with initializer
already
> > are constexpr, so this change doesn't really do anything, does it?
> 
> On this specific line, no, but it does for some of the other lines here (e.g.
> the HSL stuff, allowing kDefaultTintButtonsIncognito to reference
> kDefaultTintButtons without adding a static initializer) and I read both the

Huh, are you sure kDefaultTintButtons adds a static initializer if you don't
make this constexpr?

> Google styleguide and Effective Modern C++ as saying to use constexpr
> consistently when declaring compile-time constants, so it seemed to make sense
> to me to change all such expressions atop this file at the same time.

There are lots other of non-constexpr const SkColors in this file.

[Nico, 2016-04-13 22:54:25]

sorry, to be clear, i'm not against allowing this! i just don't understand when
we want to replace existing consts with constexprs (probably not "always"? we
already have many of these mass-replacements in progress), and this cl doesn't
have a consistent pattern for doing so as far as i can tell. maybe. maybe it
_is_ "always" though.

[danakj, 2016-04-13 23:23:20]

On Wed, Apr 13, 2016 at 3:54 PM, <thakis@chromium.org> wrote:

> sorry, to be clear, i'm not against allowing this! i just don't understand
> when
> we want to replace existing consts with constexprs (probably not "always"?
> we
> already have many of these mass-replacements in progress), and this cl
> doesn't
> have a consistent pattern for doing so as far as i can tell. maybe. maybe
> it
> _is_ "always" though.
>

I'm not really sure either. I thought for consts in headers, but vmpstr
pointed out that it seems constexprs still get storage so there's still odr
concerns. So, most definitely for functions that you want to use in
constants, otherwise.. I dunno.

constexpr seems roughly the same as const with a few bonus properties when
you need them. A mass conversion doesn't seeeeem warrented for that but I
might be missing something. This CL does a fine job of demonstrating they
compile without introducing anything too weird in ui/gfx/, but we should
sort this stuff out.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Peter Kasting, 2016-04-14 04:55:24]

On 2016/04/13 23:23:20, danakj wrote:
> On Wed, Apr 13, 2016 at 3:54 PM, <mailto:thakis@chromium.org> wrote:
> 
> > sorry, to be clear, i'm not against allowing this! i just don't understand
> > when
> > we want to replace existing consts with constexprs (probably not "always"?
> > we
> > already have many of these mass-replacements in progress), and this cl
> > doesn't
> > have a consistent pattern for doing so as far as i can tell. maybe. maybe
> > it
> > _is_ "always" though.
> >
> 
> I'm not really sure either. I thought for consts in headers, but vmpstr
> pointed out that it seems constexprs still get storage so there's still odr
> concerns. So, most definitely for functions that you want to use in
> constants, otherwise.. I dunno.

Reading the guidance elsewhere this is what I think is the right thing:
* At least for variables, anywhere you currently use "const", but could use
"constexpr" and things will still compile, prefer constexpr.  Note that if
constexpr _doesn't_ work, you might want to sanity-check that the code isn't
going to cause a static initializer.
* The benefits of the above are not so large that we should bother to
mass-convert existing code unless either the inconsistency between old code and
new bugs people, or someone demonstrates a real performance win with some
auto-generated subset of that change.
* Be cautious about constexpr functions.  Mark them as such if they're needed to
init constexpr variables, otherwise don't.

So I think the rule is "always" for variables, to the degree things will
compile, and "if necessary" for functions -- and all this applies primarily to
new code.

https://codereview.chromium.org/1885183002/diff/1/chrome/browser/themes/theme...
File chrome/browser/themes/theme_properties.cc (right):

https://codereview.chromium.org/1885183002/diff/1/chrome/browser/themes/theme...
chrome/browser/themes/theme_properties.cc:103: constexpr SkColor
kDefaultColorNTPBackground = SK_ColorWHITE;
On 2016/04/13 22:49:15, Nico wrote:
> On 2016/04/13 21:25:09, Peter Kasting wrote:
> > On 2016/04/13 21:06:12, Nico wrote:
> > > Does this help any? const variables of integral type with initializer
> already
> > > are constexpr, so this change doesn't really do anything, does it?
> > 
> > On this specific line, no, but it does for some of the other lines here
(e.g.
> > the HSL stuff, allowing kDefaultTintButtonsIncognito to reference
> > kDefaultTintButtons without adding a static initializer) and I read both the
> 
> Huh, are you sure kDefaultTintButtons adds a static initializer if you don't
> make this constexpr?

Yes, because we landed it that way originally, caused a static initializer, and
had to fix.  My comment saying "constexpr might have saved us" on the fix CL is
what I looked up to find a file where some constexprs might do some good :)

> > Google styleguide and Effective Modern C++ as saying to use constexpr
> > consistently when declaring compile-time constants, so it seemed to make
sense
> > to me to change all such expressions atop this file at the same time.
> 
> There are lots other of non-constexpr const SkColors in this file.

Yes, but sadly none of those can be changed.  SkColorSet[A]RGB() is, in debug
mode, a function which in turn includes the code in SK_ASSERT, which is a
complicated macro that calls off to printf() and the like, so I can't make it a
constexpr function.

The implication of that is that, in debug mode, every const SkColor in our
codebase that's initialized using SkColorSet[A]RGB() probably incurs a static
initializer.  I found this depressing.  Luckily, it's a simple macro in release
mode, so it only affects debug builds.

[Peter Kasting, 2016-04-14 04:56:17]

(BTW, not submitting this quite yet, in case you'd like to direct me to update
the Dos-And-Donts page or something with additional guidance when landing this,
based on what I just said.)

[Nico, 2016-04-14 11:21:59]

I think adding some of what you said to the page would be helpful. (For new
code, prefer constexpr instead of const for (global?) variables when possible.
Don't mass-convert existing code. Be cautious about constexpr functions.)

With that, lgtm, thanks for the explanation.

[Peter Kasting, 2016-04-14 17:01:31]

I added some guidance to the styleguide itself.

[Peter Kasting, 2016-04-14 17:01:37]

The CQ bit was checked by pkasting@chromium.org

[Peter Kasting, 2016-04-14 17:01:37]

The patchset sent to the CQ was uploaded after l-g-t-m from danakj@chromium.org,
thakis@chromium.org
Link to the patchset: https://codereview.chromium.org/1885183002/#ps20001
(title: "Add guidance")

[commit-bot: I haz the power, 2016-04-14 17:02:04]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1885183002/20001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1885183002/20001

[commit-bot: I haz the power, 2016-04-14 17:54:17]

Committed patchset #2 (id:20001)

[commit-bot: I haz the power, 2016-04-14 17:55:28]

Description was changed from

==========
Allow constexpr, and use it in a few places.

BUG=none
TEST=none
==========

to

==========
Allow constexpr, and use it in a few places.

BUG=none
TEST=none

Committed: https://crrev.com/ce246fab1effdfa0cc52e3954b86690f389a9e5d
Cr-Commit-Position: refs/heads/master@{#387355}
==========

[commit-bot: I haz the power, 2016-04-14 17:55:29]

Patchset 2 (id:??) landed as
https://crrev.com/ce246fab1effdfa0cc52e3954b86690f389a9e5d
Cr-Commit-Position: refs/heads/master@{#387355}