Closed Bug 924014 Opened 6 years ago Closed 6 years ago

Do not rely on std::numeric_limits<char16_t> being available

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set

Tracking

()

RESOLVED FIXED
mozilla27

People

(Reporter: ehsan, Assigned: ehsan)

References

Details

Attachments

(1 file, 1 obsolete file)

Attached patch Patch (v1) (obsolete) — Splinter Review
No description provided.
Attachment #814018 - Flags: review?(jwalden+bmo)
Attached patch Patch (v2)Splinter Review
Case insensitive filesystems suck!  This conflicts with <limits.h>. :(
Attachment #814018 - Attachment is obsolete: true
Attachment #814018 - Flags: review?(jwalden+bmo)
Attachment #814019 - Flags: review?(jwalden+bmo)
Comment on attachment 814019 [details] [diff] [review]
Patch (v2)

Review of attachment 814019 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/ctypes/CTypes.cpp
@@ +1377,5 @@
>  JS_STATIC_ASSERT(sizeof(long long) == 8);
>  JS_STATIC_ASSERT(sizeof(size_t) == sizeof(uintptr_t));
>  JS_STATIC_ASSERT(sizeof(float) == 4);
>  JS_STATIC_ASSERT(sizeof(PRFuncPtr) == sizeof(void*));
> +JS_STATIC_ASSERT(NumericLimits<double>::is_signed);

If NumericLimits is only needed in code that may allow char16_t, why do you need NumericLimits<double> here?
(In reply to comment #2)
> Comment on attachment 814019 [details] [diff] [review]
>   --> https://bugzilla.mozilla.org/attachment.cgi?id=814019
> Patch (v2)
> 
> Review of attachment 814019 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/src/ctypes/CTypes.cpp
> @@ +1377,5 @@
> >  JS_STATIC_ASSERT(sizeof(long long) == 8);
> >  JS_STATIC_ASSERT(sizeof(size_t) == sizeof(uintptr_t));
> >  JS_STATIC_ASSERT(sizeof(float) == 4);
> >  JS_STATIC_ASSERT(sizeof(PRFuncPtr) == sizeof(void*));
> > +JS_STATIC_ASSERT(NumericLimits<double>::is_signed);
> 
> If NumericLimits is only needed in code that may allow char16_t, why do you
> need NumericLimits<double> here?

For consistency in this file.
Comment on attachment 814019 [details] [diff] [review]
Patch (v2)

Review of attachment 814019 [details] [diff] [review]:
-----------------------------------------------------------------

::: mfbt/NumericLimits.h
@@ +19,5 @@
> +/**
> + * The NumericLimits class provides a compatibility layer with std::numeric_limits
> + * for char16_t, otherwise it is exactly the same as std::numeric_limits.
> + * Code which does not need std::numeric_limits<char16_t> should avoid using
> + * NumericLimits.

I'm not sure I agree with this bit, exactly, but meh.

@@ +21,5 @@
> + * for char16_t, otherwise it is exactly the same as std::numeric_limits.
> + * Code which does not need std::numeric_limits<char16_t> should avoid using
> + * NumericLimits.
> + */
> +template<class T>

typename T, since this won't always be a class (stylistic nit, I know they're equally functional here :-) ).
Attachment #814019 - Flags: review?(jwalden+bmo) → review+
https://hg.mozilla.org/mozilla-central/rev/7d0c0b0b8913
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla27
Depends on: 1288603
You need to log in before you can comment on or make changes to this bug.