Bug 201879 – Use constexpr instead of const in symbol definitions that are obviously constexpr.

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Mark Lam]

...

[Mark Lam]

Created attachment 378981 [details]
proposed patch.

Let's get some EWS testing first.

[EWS Watchlist]

This patch modifies the inspector protocol generator. Please ensure that you have rebaselined any generator test results (i.e., by running `Tools/Scripts/run-inspector-generator-tests --reset-results`)

[Joseph Pecoraro]

Can this include the reason "why"?

[Mark Lam]

Created attachment 378982 [details]
proposed patch.

[Mark Lam]

Created attachment 378985 [details]
proposed patch.

[Joseph Pecoraro]

Comment on attachment 378985 [details]
proposed patch.

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

rs=me, waiting for tests

> Source/JavaScriptCore/ChangeLog:11
> +        const may require external storage  (at the compiler's whim) though these
> +        currently do not.  constexpr makes it clear that the value is a literal constant
> +        that can be inlined.  In most cases in the code, when we say static const, we
> +        actually mean static constexpr.  I'm changing the code to reflect this.

Nice! Does that mean a binary size (and DATA) reduction? Do we have a measurement?

> Source/JavaScriptCore/testRegExp.cpp:2
> + *  Copyright (C) 2011-2019 Apple Inc. All rights reserved.

We probably don't need to update the copyrights for these kind of trivial changes. But since you've already done it that seems fine.

[Mark Lam]

(In reply to Joseph Pecoraro from comment #6)
> Comment on attachment 378985 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=378985&action=review
> 
> rs=me, waiting for tests

Thanks.

> > Source/JavaScriptCore/ChangeLog:11
> > +        const may require external storage  (at the compiler's whim) though these
> > +        currently do not.  constexpr makes it clear that the value is a literal constant
> > +        that can be inlined.  In most cases in the code, when we say static const, we
> > +        actually mean static constexpr.  I'm changing the code to reflect this.
> 
> Nice! Does that mean a binary size (and DATA) reduction? Do we have a
> measurement?

I don't expect any size change, and I don't plan to spend time measuring it.  We'll let a bot measure the size difference (if any) later.

[Yusuke Suzuki]

I like this change. Except for `static const int`, other definition in class-scope can potentially allocate storage. And it sometimes causes link failure. And we sometimes avoid such a link failure by using a hack like,

class A {
public:
    static unsigned value = 42;
};

In otehr .cpp file
use(+A::value);  // Use + to avoid linking to A::value storage.

I don't remember it but I know we had at least one such a hack for MacroAssembler's some register variable.

If we use constexpr, it becomes C++17 inline variable, then, the above problem does not happen.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #8)
> I like this change. Except for `static const int`, other definition in
> class-scope can potentially allocate storage. And it sometimes causes link
> failure. And we sometimes avoid such a link failure by using a hack like,
> 
> class A {
> public:
>     static unsigned value = 42;
> };
> 
> In otehr .cpp file
> use(+A::value);  // Use + to avoid linking to A::value storage.
> 
> I don't remember it but I know we had at least one such a hack for
> MacroAssembler's some register variable.
> 
> If we use constexpr, it becomes C++17 inline variable, then, the above
> problem does not happen.

Yes, b3/B3Common.cpp's `return static_cast<GPRReg>(+MacroAssembler::dataTempRegister);`. We can remove such a hack.

[Mark Lam]

(In reply to Yusuke Suzuki from comment #9)
> (In reply to Yusuke Suzuki from comment #8)
> > I like this change. Except for `static const int`, other definition in
> > class-scope can potentially allocate storage. And it sometimes causes link
> > failure. And we sometimes avoid such a link failure by using a hack like,
> > 
> > class A {
> > public:
> >     static unsigned value = 42;
> > };
> > 
> > In otehr .cpp file
> > use(+A::value);  // Use + to avoid linking to A::value storage.
> > 
> > I don't remember it but I know we had at least one such a hack for
> > MacroAssembler's some register variable.
> > 
> > If we use constexpr, it becomes C++17 inline variable, then, the above
> > problem does not happen.
> 
> Yes, b3/B3Common.cpp's `return
> static_cast<GPRReg>(+MacroAssembler::dataTempRegister);`. We can remove such
> a hack.

Let's do this in a follow up patch. Thanks.

[Mark Lam]

Landed in r250005: <http://trac.webkit.org/r250005>.

[Radar WebKit Bug Importer]

<rdar://problem/55522340>