...
Created attachment 378981 [details] proposed patch. Let's get some EWS testing first.
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`)
Can this include the reason "why"?
Created attachment 378982 [details] proposed patch.
Created attachment 378985 [details] proposed patch.
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.
(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.
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.
(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.
(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.
Landed in r250005: <http://trac.webkit.org/r250005>.
<rdar://problem/55522340>