Bug 201879 - Use constexpr instead of const in symbol definitions that are obviously constexpr.
Summary: Use constexpr instead of const in symbol definitions that are obviously const...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-09-17 12:43 PDT by Mark Lam
Modified: 2019-09-19 09:31 PDT (History)
14 users (show)

See Also:


Attachments
proposed patch. (558.76 KB, patch)
2019-09-17 12:49 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (559.37 KB, patch)
2019-09-17 13:26 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (559.71 KB, patch)
2019-09-17 13:45 PDT, Mark Lam
joepeck: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2019-09-17 12:43:19 PDT
...
Comment 1 Mark Lam 2019-09-17 12:49:13 PDT
Created attachment 378981 [details]
proposed patch.

Let's get some EWS testing first.
Comment 2 EWS Watchlist 2019-09-17 12:49:43 PDT
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`)
Comment 3 Joseph Pecoraro 2019-09-17 13:18:06 PDT
Can this include the reason "why"?
Comment 4 Mark Lam 2019-09-17 13:26:23 PDT
Created attachment 378982 [details]
proposed patch.
Comment 5 Mark Lam 2019-09-17 13:45:12 PDT
Created attachment 378985 [details]
proposed patch.
Comment 6 Joseph Pecoraro 2019-09-17 14:04:13 PDT
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.
Comment 7 Mark Lam 2019-09-17 14:07:15 PDT
(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.
Comment 8 Yusuke Suzuki 2019-09-17 14:11:30 PDT
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.
Comment 9 Yusuke Suzuki 2019-09-17 14:14:47 PDT
(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.
Comment 10 Mark Lam 2019-09-17 14:17:47 PDT
(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.
Comment 11 Mark Lam 2019-09-19 09:31:00 PDT
Landed in r250005: <http://trac.webkit.org/r250005>.
Comment 12 Radar WebKit Bug Importer 2019-09-19 09:31:19 PDT
<rdar://problem/55522340>