Closed Bug 1451278 Opened 6 years ago Closed 6 years ago

Remove ConstExpr* variants that worked around the lack of C++14 constexpr

Categories

(Core :: JavaScript Engine, enhancement, P3)

enhancement

Tracking

()

RESOLVED FIXED
mozilla61

People

(Reporter: emk, Assigned: n.nethercote)

References

Details

Attachments

(2 files)

Now we can use C++14 constexpr everywhere.
Assignee: nobody → n.nethercote
Component: General → MFBT
Oh, there's the IsAscii stuff as well, which is outside MFBT.
Component: MFBT → General
No longer blocks: 1449082
Oops, I splitted the IsAscii bit right now. By the way, are you already working on? I have a patch.
emk: oh, sorry! I didn't see that comment until just now. Hopefully my patch looks the same as yours?
Try push: https://treeherder.mozilla.org/#/jobs?repo=try&revision=bcd8e01989d987268cfb6beb7f86e948eae3730d

Note that the spidermonkey-sm-rust-bindings-linux64/debug job is failing due to these changes. That's a tier 2 job, I don't know if it's a blocker or not.
Comment on attachment 8965173 [details]
Bug 1451278 - Remove ConstExpr hash functions.

https://reviewboard.mozilla.org/r/233858/#review239524

::: mfbt/HashFunctions.h:217
(Diff revision 1)
>  template<typename T>
> -uint32_t
> +constexpr uint32_t
>  HashUntilZero(const T* aStr)
>  {
>    uint32_t hash = 0;
> -  for (T c; (c = *aStr); aStr++) {
> +  for (T c = 0; (c = *aStr); aStr++) {

`for (; T c = *aStr; aStr++) {` is possible, although I'm not sure if this is better than a dummy initial value.
Depends on: 1451931
Comment on attachment 8965173 [details]
Bug 1451278 - Remove ConstExpr hash functions.

https://reviewboard.mozilla.org/r/233858/#review240594

::: mfbt/HashFunctions.h:217
(Diff revision 1)
>  template<typename T>
> -uint32_t
> +constexpr uint32_t
>  HashUntilZero(const T* aStr)
>  {
>    uint32_t hash = 0;
> -  for (T c; (c = *aStr); aStr++) {
> +  for (T c = 0; (c = *aStr); aStr++) {

I like Kimura-san's version slightly better than the current code or the proposed patch, even if it made me do a double-take: no questions about whether `c` is accessed uninitialized or whether a `0` character is tacked on to the beginning.  Up to you, though.
Attachment #8965173 - Flags: review?(nfroyd) → review+
Comment on attachment 8965174 [details]
Bug 1451278 - Remove NS_ConstExprIsAscii() functions.

https://reviewboard.mozilla.org/r/233860/#review240596

::: xpcom/base/nsCRTGlue.h:107
(Diff revision 1)
>  constexpr bool
> -NS_ConstExprIsAscii(const char16_t* aString)
> +NS_IsAscii(const char16_t* aString)

Because I don't yet have a good intuition about these kind of functions, do you know what happens if the compiler can't constant-fold these away?  Do the functions always get inlined at the point of use, or do they get treated in a similar fashion to templated functions, where every compilation unit gets its own copy, and then only one gets kept at the end?  The latter seems OK, the former not so much...

Ah, the standard says "[A function or static data member declared with the `constexpr` specifier is implicitly an inline function or variable ](http://eel.is/c++draft/dcl.constexpr#1).  That doesn't seem particularly desirable in this case...
Attachment #8965174 - Flags: review?(nfroyd) → review+
> Because I don't yet have a good intuition about these kind of functions, do
> you know what happens if the compiler can't constant-fold these away?  Do
> the functions always get inlined at the point of use, or do they get treated
> in a similar fashion to templated functions, where every compilation unit
> gets its own copy, and then only one gets kept at the end? 

My experience is that if the compiler cannot compute a constexpr expression at compile-time it gives a compile error. (I think that's what you're asking?) I.e. unlike a lot of other things (such as 'inline'), 'constexpr' is a requirement that this be evaluated at compile time, not just a suggestion.
(In reply to Nicholas Nethercote [:njn] from comment #11)
> > Because I don't yet have a good intuition about these kind of functions, do
> > you know what happens if the compiler can't constant-fold these away?  Do
> > the functions always get inlined at the point of use, or do they get treated
> > in a similar fashion to templated functions, where every compilation unit
> > gets its own copy, and then only one gets kept at the end? 
> 
> My experience is that if the compiler cannot compute a constexpr expression
> at compile-time it gives a compile error. (I think that's what you're
> asking?) I.e. unlike a lot of other things (such as 'inline'), 'constexpr'
> is a requirement that this be evaluated at compile time, not just a
> suggestion.

I don't see any way that, say, attachment 8966312 [details] [diff] [review] (bug 1452619) could work if that was the case.  Maybe we're not using the same terminology to describe what we want?

If `constexpr` meant that the compiler was *required* to do compile-time evaluation (and error if it could not), wouldn't that force us into precisely the world you're trying to remove here?  That is, we'd *have* to have separate `constexpr` functions and non-`constexpr` functions, and always use the correct one, lest we have compile-time errors.
constexpr variables are required to be evaluated at compile-time, but constexpr functions are not. If onstexpr functions are not evaluated at compile-time, it will be treated like inline functions.
Thank you for the clarification, emk!
Comment on attachment 8965174 [details]
Bug 1451278 - Remove NS_ConstExprIsAscii() functions.

https://reviewboard.mozilla.org/r/233860/#review242350

::: commit-message-5c33f:3
(Diff revision 1)
> +Bug 1451278 - Remove NS_ConstExprIsAscii() functions. r=froydnj
> +
> +MozReview-Commit-ID: 9KfPrXPYJaI

I added one reference to NS_ConstExprIsAscii here
https://hg.mozilla.org/integration/autoland/rev/4d0b4650e438#l1.43
to land bug 1445601 before this bug. Please do not forget replace it when landing this patch.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Backed out 2 changesets (bug 1451278) for breaking windows pgo builds

Backout: https://treeherder.mozilla.org/#/jobs?repo=mozilla-central&revision=6a87ef9c1acc5f6af848c00b8b9f608444ff8a72
Flags: needinfo?(n.nethercote)
Assignee: n.nethercote → nobody
Flags: needinfo?(n.nethercote)
I reopened bug 1451535 and relanded the NS_IsAscii part because it did not break PGO.
Component: General → JavaScript Engine
Priority: -- → P3
I opened bug 1480660 to land the first patch.
Status: REOPENED → RESOLVED
Closed: 6 years ago6 years ago
Resolution: --- → FIXED
Assignee: nobody → n.nethercote
You need to log in before you can comment on or make changes to this bug.