Closed Bug 1449082 Opened 6 years ago Closed 6 years ago

Make NS_IsAscii constexpr

Categories

(Core :: XPCOM, enhancement)

Product:
Core
Component:
XPCOM
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla61
Tracking Flags:
Tracking Status
firefox61 --- fixed

People

(Reporter: emk, Assigned: emk)

References

Details

Attachments

(1 file)

Bug 1449082 - Make NS_IsAscii constexpr.
59 bytes, text/x-review-board-request
froydnj
: review+
Details 
To make it usable in static asserts.
Assignee: nobody → VYV03354
Status: NEW → ASSIGNED
Comment on attachment 8962742 [details]
Bug 1449082 - Make NS_IsAscii constexpr.

https://reviewboard.mozilla.org/r/231608/#review237160

I have concerns, but am happy to hear alternative points of view on this.

::: xpcom/base/nsCRTGlue.h:101
(Diff revision 1)
> -bool NS_IsAscii(char16_t aChar);
> -bool NS_IsAscii(const char16_t* aString);
> -bool NS_IsAsciiAlpha(char16_t aChar);
> -bool NS_IsAsciiDigit(char16_t aChar);
> -bool NS_IsAsciiWhitespace(char16_t aChar);
> +#if __cpp_constexpr >= 201304 || defined(_MSC_VER)
> +#define MOZ_CXX14_CONSTEXPR constexpr
> +#else
> +#define MOZ_CXX14_CONSTEXPR /* constexpr */
> +#endif

I don't have strong opinions against this change, but it seems that this bit renders a lot of the changes useless for the desired outcome of using the functions in `static_assert`: We can't use such `static_assert`s in any places where `MOZ_CXX14_constexpr` isn't actually `constexpr`.  I mean, we could `#ifdef` around this at the `static_assert` callsites, but then we're missing assertions on half of our platforms.  I'm not sure that's a winning strategy.

Also, is the `_MSC_VER` restriction due to the compiler erroring our about "too many steps required" or somesuch when evaluating `constexpr` functions?  njn added some option to moz.build files explicitly for dealing with that (`-ZconstexprExternSteps` or somesuch?), and dmajor was wondering if we shouldn't just apply that option globally.

These changes also induce these functions where they weren't inlined before, correct?  So there's some codebloat associated with this change if so?  (Granted, it looks like these functions are not widely used, so it's probably not severe bloat, but still...)  I know for other, similar changes that njn did recently for static atomics, we elected to use separately-named `constexpr` functions, which is one route to dealing with that.
Attachment #8962742 - Flags: review?(nfroyd)
njn and dmajor may be interested to hear of difficulties with MSVC and constexpr; see comment 2.
Comment on attachment 8962742 [details]
Bug 1449082 - Make NS_IsAscii constexpr.

https://reviewboard.mozilla.org/r/231608/#review237160

> I don't have strong opinions against this change, but it seems that this bit renders a lot of the changes useless for the desired outcome of using the functions in `static_assert`: We can't use such `static_assert`s in any places where `MOZ_CXX14_constexpr` isn't actually `constexpr`.  I mean, we could `#ifdef` around this at the `static_assert` callsites, but then we're missing assertions on half of our platforms.  I'm not sure that's a winning strategy.
> 
> Also, is the `_MSC_VER` restriction due to the compiler erroring our about "too many steps required" or somesuch when evaluating `constexpr` functions?  njn added some option to moz.build files explicitly for dealing with that (`-ZconstexprExternSteps` or somesuch?), and dmajor was wondering if we shouldn't just apply that option globally.
> 
> These changes also induce these functions where they weren't inlined before, correct?  So there's some codebloat associated with this change if so?  (Granted, it looks like these functions are not widely used, so it's probably not severe bloat, but still...)  I know for other, similar changes that njn did recently for static atomics, we elected to use separately-named `constexpr` functions, which is one route to dealing with that.

> I don't have strong opinions against this change, but it seems that this bit renders a lot of the changes useless for the desired outcome of using the functions in static_assert: We can't use such static_asserts in any places where MOZ_CXX14_constexpr isn't actually constexpr.  I mean, we could #ifdef around this at the static_assert callsites, but then we're missing assertions on half of our platforms.  I'm not sure that's a winning strategy.

This #ifdef is temporary. I only require C++14 constexpr in the Windows-specific code path (see bug 1445601), so this is sufficient at the moment. I would not like to wait for bug 1449066 that will require non-trivial change.

> Also, is the _MSC_VER restriction due to the compiler erroring our about "too many steps required" or somesuch when evaluating constexpr functions?  njn added some option to moz.build files explicitly for dealing with that (-ZconstexprExternSteps or somesuch?), and dmajor was wondering if we shouldn't just apply that option globally.

I did not restrict MSVC. I *allowed* MSVC because I need C++14 constexpr in the Windows-specific code path. (Note that "||" means logical OR, just in case.) I had to add the MSVC-specific macro because MSVC does not support SD6 feature testing macro.
Comment on attachment 8962742 [details]
Bug 1449082 - Make NS_IsAscii constexpr.

https://reviewboard.mozilla.org/r/231608/#review237160

> > I don't have strong opinions against this change, but it seems that this bit renders a lot of the changes useless for the desired outcome of using the functions in static_assert: We can't use such static_asserts in any places where MOZ_CXX14_constexpr isn't actually constexpr.  I mean, we could #ifdef around this at the static_assert callsites, but then we're missing assertions on half of our platforms.  I'm not sure that's a winning strategy.
> 
> This #ifdef is temporary. I only require C++14 constexpr in the Windows-specific code path (see bug 1445601), so this is sufficient at the moment. I would not like to wait for bug 1449066 that will require non-trivial change.
> 
> > Also, is the _MSC_VER restriction due to the compiler erroring our about "too many steps required" or somesuch when evaluating constexpr functions?  njn added some option to moz.build files explicitly for dealing with that (-ZconstexprExternSteps or somesuch?), and dmajor was wondering if we shouldn't just apply that option globally.
> 
> I did not restrict MSVC. I *allowed* MSVC because I need C++14 constexpr in the Windows-specific code path. (Note that "||" means logical OR, just in case.) I had to add the MSVC-specific macro because MSVC does not support SD6 feature testing macro.

Ah, indeed, I botched reading the MSVC condition.  My mistake!
What is the SD6 feature testing macro?

When I did similar stuff in bug 1443706, GCC 4.9 gave me problems, because it requires constexpr functions to contain a single expression. The solution was to introduce alternative versions of the functions that used recursion, in order to make them contain a single expression.

Have you done a try push with this patch? I suspect the static analysis jobs will fail because they still use GCC 4.9. jgilbert is in the process of upgrading our minimum GCC to 6.1. See bug 1444271 and bug 1444274 for details.
(In reply to Nicholas Nethercote [:njn] from comment #6)
> What is the SD6 feature testing macro?

`__cpp_constexpr >= 201304` is an example of the macro. More generally:
https://isocpp.org/std/standing-documents/sd-6-sg10-feature-test-recommendations

> When I did similar stuff in bug 1443706, GCC 4.9 gave me problems, because
> it requires constexpr functions to contain a single expression. The solution
> was to introduce alternative versions of the functions that used recursion,
> in order to make them contain a single expression.

Makes sense.

> Have you done a try push with this patch? I suspect the static analysis jobs
> will fail because they still use GCC 4.9. jgilbert is in the process of
> upgrading our minimum GCC to 6.1. See bug 1444271 and bug 1444274 for
> details.

I ran a try push and found an error. I will submit a new patch shortly with the review request disabled.
If you can wait until GCC 6.1 is required, I *think* you'll be able to remove all the conditional stuff.
For my purpose, I can just #ifdef-out static_asserts for older compilers.
I added NS_ConstExpr* functions in the latest patch.
Comment on attachment 8962742 [details]
Bug 1449082 - Make NS_IsAscii constexpr.

https://reviewboard.mozilla.org/r/231608/#review237684

I'm not keen on having NS_ConstExprStrLen and its use in NS_ConstExprIsAscii, see below.

::: xpcom/base/nsCRTGlue.h:152
(Diff revision 3)
> +// and NS_IsAscii should be made `constexpr`.
> +constexpr bool
> +NS_ConstExprIsAscii(const char16_t* aString, uint32_t aLength)
> +{
> +  return aLength == 0 ? true :
> +    aLength == 1 ? *aString <= 0x7F :

Why not use `NS_IsAscii` here?

::: xpcom/base/nsCRTGlue.h:153
(Diff revision 3)
> +    NS_ConstExprIsAscii(aString, aLength / 2) &&
> +    NS_ConstExprIsAscii(aString + aLength / 2, aLength - aLength / 2);

Is this really better than the obvious recursive code?  Does this cleverly work around some constexpr evaluation limits on some compilers or something?

Doing things this way requires having a quite complicated NS_ConstExprStrLen, so unless the straightforward recursive code fails, I'm inclined to just use the straightforward code instead.

::: xpcom/base/nsCRTGlue.h:162
(Diff revision 3)
> +    NS_ConstExprIsAscii(aString, aLength / 2) &&
> +    NS_ConstExprIsAscii(aString + aLength / 2, aLength - aLength / 2);

Same question here.
Attachment #8962742 - Flags: review?(nfroyd)
(In reply to Nathan Froyd [:froydnj] from comment #13)
> ::: xpcom/base/nsCRTGlue.h:153
> (Diff revision 3)
> > +    NS_ConstExprIsAscii(aString, aLength / 2) &&
> > +    NS_ConstExprIsAscii(aString + aLength / 2, aLength - aLength / 2);
> 
> Is this really better than the obvious recursive code?  Does this cleverly
> work around some constexpr evaluation limits on some compilers or something?

This is to keep the recursion depth to lower (O(logN)) than the obvious code (O(N)). But I'm OK with the simpler one if you feel this is over-engineering because constexpr functions are temporary anyway.
(In reply to Masatoshi Kimura [:emk] from comment #14)
> (In reply to Nathan Froyd [:froydnj] from comment #13)
> > ::: xpcom/base/nsCRTGlue.h:153
> > (Diff revision 3)
> > > +    NS_ConstExprIsAscii(aString, aLength / 2) &&
> > > +    NS_ConstExprIsAscii(aString + aLength / 2, aLength - aLength / 2);
> > 
> > Is this really better than the obvious recursive code?  Does this cleverly
> > work around some constexpr evaluation limits on some compilers or something?
> 
> This is to keep the recursion depth to lower (O(logN)) than the obvious code
> (O(N)). But I'm OK with the simpler one if you feel this is over-engineering
> because constexpr functions are temporary anyway.

I think if we can get away with the O(N) recursion depth, we should just do that.
Comment on attachment 8962742 [details]
Bug 1449082 - Make NS_IsAscii constexpr.

https://reviewboard.mozilla.org/r/231608/#review237684

> Why not use `NS_IsAscii` here?

Using NS_IsAscii.

> Is this really better than the obvious recursive code?  Does this cleverly work around some constexpr evaluation limits on some compilers or something?
> 
> Doing things this way requires having a quite complicated NS_ConstExprStrLen, so unless the straightforward recursive code fails, I'm inclined to just use the straightforward code instead.

Changed to the ovbious one.
Comment on attachment 8962742 [details]
Bug 1449082 - Make NS_IsAscii constexpr.

https://reviewboard.mozilla.org/r/231608/#review238002

Thanks!
Attachment #8962742 - Flags: review?(nfroyd) → review+
Pushed by VYV03354@nifty.ne.jp:
https://hg.mozilla.org/integration/autoland/rev/89678976aa89
Make NS_IsAscii constexpr. r=froydnj
https://hg.mozilla.org/mozilla-central/rev/89678976aa89
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
status-firefox61: affected → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla61
Depends on: 1451278
Depends on: 1451535
No longer depends on: 1451278
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: