hit a little more stuff with constexpr to fix some static constructors

RESOLVED FIXED in mozilla28

Status

()

RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: tbsaunde, Assigned: tbsaunde)

Tracking

unspecified
mozilla28
x86_64
Linux
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [qa-])

Attachments

(2 attachments, 1 obsolete attachment)

Comment hidden (empty)
(Assignee)

Comment 1

5 years ago
Created attachment 831373 [details] [diff] [review]
bug 938025 - some static constructors
Attachment #831373 - Flags: review?(bjacob)
Comment on attachment 831373 [details] [diff] [review]
bug 938025 - some static constructors

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

::: gfx/layers/FrameMetrics.h
@@ +36,5 @@
>    // We use IDs to identify frames across processes.
>    typedef uint64_t ViewID;
>    static const ViewID NULL_SCROLL_ID;   // This container layer does not scroll.
>    static const ViewID ROOT_SCROLL_ID;   // This is the root scroll frame.
> +  static const ViewID START_SCROLL_ID = 2;  // This is the ID that scrolling subframes

This seems unrelated to the rest of this patch, r-.

::: gfx/layers/Layers.cpp
@@ -37,5 @@
>  
>  typedef FrameMetrics::ViewID ViewID;
>  const ViewID FrameMetrics::NULL_SCROLL_ID = 0;
>  const ViewID FrameMetrics::ROOT_SCROLL_ID = 1;
> -const ViewID FrameMetrics::START_SCROLL_ID = 2;

This seems unrelated to the rest of this patch, r-.
Attachment #831373 - Flags: review?(bjacob) → review-
(Assignee)

Comment 3

5 years ago
(In reply to Benoit Jacob [:bjacob] from comment #2)
> Comment on attachment 831373 [details] [diff] [review]
> bug 938025 - some static constructors
> 
> Review of attachment 831373 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/FrameMetrics.h
> @@ +36,5 @@
> >    // We use IDs to identify frames across processes.
> >    typedef uint64_t ViewID;
> >    static const ViewID NULL_SCROLL_ID;   // This container layer does not scroll.
> >    static const ViewID ROOT_SCROLL_ID;   // This is the root scroll frame.
> > +  static const ViewID START_SCROLL_ID = 2;  // This is the ID that scrolling subframes
> 
> This seems unrelated to the rest of this patch, r-.

err, forgot about that bit, but actually it was done for the same reason to get rid of a static initializer somewhere.  What happen is this, somefile did
static const int something = FrameMetrics::VIEW_ID;
So the compiler created a static initializer to copy the value of VIEW_ID to something because it couldn't tell at compile time what that was.  Which is oh so sad making *sigh*
(Assignee)

Comment 4

5 years ago
Created attachment 8335478 [details] [diff] [review]
bug 938025 - some static constructors

the RootingAPI.h changes are necessary because of js/src/vm/Id.cpp which calls FromMarkedLocation on a global.
The FrameMetrics change is to allow another constant based on it to be computable at compile time.

waldo for the js stuff
bjacob for the gfx/ stuff
bz for the other odds and ends
Attachment #831373 - Attachment is obsolete: true
Attachment #8335478 - Flags: review?(jwalden+bmo)
Attachment #8335478 - Flags: review?(bzbarsky)
Attachment #8335478 - Flags: review?(bjacob)
Comment on attachment 8335478 [details] [diff] [review]
bug 938025 - some static constructors

Why do you need to expand out NS_DECL_ISUPPORTS?
And if the key is that you want the MOZ_OVERRIDE, then we should just fix the codegenerator for xpidl to do that automatically, I think.
(Assignee)

Comment 7

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #5)
> Comment on attachment 8335478 [details] [diff] [review]
> bug 938025 - some static constructors
> 
> Why do you need to expand out NS_DECL_ISUPPORTS?

I don't want the nsAutoRefCnt since AddRef / Release just return a constant and the nsAutoRefcnt constructor isn't constexpr.
Ah.  So you're looking for NS_DECL_ISUPPORTS_INHERITED, then.
(Assignee)

Comment 9

5 years ago
(In reply to Boris Zbarsky [:bz] from comment #8)
> Ah.  So you're looking for NS_DECL_ISUPPORTS_INHERITED, then.

uh, I guess that actually works, I'd forgotten we always over ride AddRef / Release and didn't think about it because this isn't really inheriting from anything.
Comment on attachment 8335478 [details] [diff] [review]
bug 938025 - some static constructors

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

::: gfx/layers/FrameMetrics.h
@@ +36,5 @@
>    // We use IDs to identify frames across processes.
>    typedef uint64_t ViewID;
>    static const ViewID NULL_SCROLL_ID;   // This container layer does not scroll.
>    static const ViewID ROOT_SCROLL_ID;   // This is the root scroll frame.
> +  static const ViewID START_SCROLL_ID = 2;  // This is the ID that scrolling subframes

This looks like an unintentional change in this patch!
Attachment #8335478 - Flags: review?(bjacob) → review-
(Assignee)

Comment 11

5 years ago
(In reply to Benoit Jacob [:bjacob] from comment #10)
> Comment on attachment 8335478 [details] [diff] [review]
> bug 938025 - some static constructors
> 
> Review of attachment 8335478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: gfx/layers/FrameMetrics.h
> @@ +36,5 @@
> >    // We use IDs to identify frames across processes.
> >    typedef uint64_t ViewID;
> >    static const ViewID NULL_SCROLL_ID;   // This container layer does not scroll.
> >    static const ViewID ROOT_SCROLL_ID;   // This is the root scroll frame.
> > +  static const ViewID START_SCROLL_ID = 2;  // This is the ID that scrolling subframes
> 
> This looks like an unintentional change in this patch!

no, read comment 3 and comment 4
(Assignee)

Updated

5 years ago
Attachment #8335478 - Flags: review- → review?(bjacob)
Comment on attachment 8335478 [details] [diff] [review]
bug 938025 - some static constructors

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

Ah, OK, sorry.
Attachment #8335478 - Flags: review?(bjacob) → review+
(Assignee)

Comment 13

5 years ago
Created attachment 8335641 [details] [diff] [review]
bug 938025 - some static constructors

use the NS_DECL_ISUPPORTS_INHERITED hack
Attachment #8335641 - Flags: review?(jwalden+bmo)
Attachment #8335641 - Flags: review?(bzbarsky)
Comment on attachment 8335641 [details] [diff] [review]
bug 938025 - some static constructors

r=me on my bits
Attachment #8335641 - Flags: review?(bzbarsky) → review+
Attachment #8335478 - Flags: review?(bzbarsky)
Comment on attachment 8335641 [details] [diff] [review]
bug 938025 - some static constructors

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

r+ with a tweak or two, and with the small but rather important changes noted to RootingAPI.h.

::: content/html/content/src/HTMLTrackElement.cpp
@@ +61,5 @@
>  namespace mozilla {
>  namespace dom {
>  
>  // The default value for kKindTable is "subtitles"
> +static MOZ_CONSTEXPR const char* kKindTableDefaultString = kKindTable->tag;

I think we added MOZ_CONSTEXPR_VAR for const variables.  That lets you kill the const here, and it also prevents warnings with some compilers for constexpr *and* const both on the same variable.  (Which was not legal at one time but became so later in C++11, or was legal but warned, or something.  Point is, MOZ_CONSTEXPR const is bad.  :-) )

::: content/html/content/src/HTMLTrackElement.h
@@ +21,5 @@
>  namespace mozilla {
>  namespace dom {
>  
>  // Map html attribute string values to TextTrackKind enums.
> +static MOZ_CONSTEXPR nsAttrValue::EnumTable kKindTable[] = {

MOZ_CONSTEXPR_VAR

::: js/public/RootingAPI.h
@@ +479,5 @@
>      void repoint(const Handle &rhs) { ptr = rhs.address(); }
>  
>    private:
>      Handle() {}
> +    MOZ_CONSTEXPR Handle(const T *p) : ptr(p) {}

So, adding a constructor like this -- even privately -- is kind of asking for trouble.  It's pretty similar to the other Handle constructors in that it can take one argument, and it'd be really pretty easy to accidentally call this when you meant to call another one.  The results of doing so would be very unsafe!  (And private doesn't save you here, because that still lets the whole of Handle use it, and also MutableHandle would see it, too, because friend.  I don't really want to have to trust both classes to carefully never call this except when it's okay -- unsafe stuff should be big and blaring as fromMarkedLocation is.)  Basically exactly the same concerns would apply to this constructor, as apply to fromMarkedLocation.

But we should be able to address these concerns without too much trouble -- we just need to gussy up the overload so that calling it means you *really* meant to get this exact constructor, and no other.  So let's do this:

  static MOZ_CONSTEXPR Handle fromMarkedLocation(const T *p) {
    return Handle(p, DeliberatelyChoosingThisOverload, ImUsingThisOnlyInFromFromMarkedLocation);
  }

  enum Disambiguator { DeliberatelyChoosingThisOverload = 42 };
  enum CallerIdentity { ImUsingThisOnlyInFromFromMarkedLocation = 17 };
  MOZ_CONSTEXPR Handle(const T *p, Disambiguator, CallerIdentity) : ptr(p) {}

If you're calling the method in this case, it's only ever going to be because someone really really meant to.  :-)
Attachment #8335641 - Flags: review?(jwalden+bmo) → review+
Comment on attachment 8335478 [details] [diff] [review]
bug 938025 - some static constructors

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

This...looks almost like a carbon copy of the previous patch?  Reflag if you really want me looking at this patch, please, beyond my saying "do what I said re the other patch".

::: content/html/content/src/HTMLTrackElement.cpp
@@ +61,5 @@
>  namespace mozilla {
>  namespace dom {
>  
>  // The default value for kKindTable is "subtitles"
> +static MOZ_CONSTEXPR const char* kKindTableDefaultString = kKindTable->tag;

MOZ_CONSTEXPR_VAR
Attachment #8335478 - Flags: review?(jwalden+bmo)
https://hg.mozilla.org/mozilla-central/rev/93370e831032
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla28
(Assignee)

Comment 18

5 years ago
(In reply to Jeff Walden [:Waldo] (remove +bmo to email) from comment #16)
> Comment on attachment 8335478 [details] [diff] [review]
> bug 938025 - some static constructors
> 
> Review of attachment 8335478 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> This...looks almost like a carbon copy of the previous patch?  Reflag if you
> really want me looking at this patch, please, beyond my saying "do what I
> said re the other patch".

indeed, apparently the r? on the old one didn't get canceled :/ sorry!
(Assignee)

Updated

5 years ago
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
https://hg.mozilla.org/mozilla-central/rev/ca137ceb2877
Status: REOPENED → RESOLVED
Last Resolved: 5 years ago5 years ago
Resolution: --- → FIXED

Updated

5 years ago
Whiteboard: [qa-]
You need to log in before you can comment on or make changes to this bug.