909514 - Include <new> before mozilla::Maybe (and move Maybe into mfbt/Maybe.h)

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Justin Lebar (not reading bugmail)]

mozilla::Maybe uses placement new, so it should include <new>.

In order to fix this, I need to move Maybe into its own file, because some random C++ test includes Util.h, and the compiler complains about infallible new when I use <new> in that file.  I figure we want to do this anyway.

Comment 1

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v1

Comment 2

[Justin Lebar (not reading bugmail)]

Attachment: Patch, v2

Whoops, forgot to include the removal of Maybe from Util.h in the previous patch.

Comment 3

[Jeff Walden [:Waldo]]

Comment on attachment 795670 [details] [diff] [review]
Patch, v2

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

::: js/src/jsproxy.h
@@ +8,5 @@
>  #define jsproxy_h
>  
>  #include "jsapi.h"
>  #include "jsfriendapi.h"
> +#include "mozilla/Maybe.h"

This should move up above the jsapi.h include, into its own include-section.

::: mfbt/Maybe.h
@@ +3,5 @@
> +/* This Source Code Form is subject to the terms of the Mozilla Public
> + * License, v. 2.0. If a copy of the MPL was not distributed with this
> + * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
> +
> +/* XXX */

/* A class for lazily constructing an object with stack-based lifetime. */

?  (although I'm not 100% certain Maybe is only used for stacked stuff, in practice -- probably this is something we should clarify with MOZ_STACK_CLASS if so)

@@ +7,5 @@
> +/* XXX */
> +
> +#ifndef mozilla_Maybe_h
> +#define mozilla_Maybe_h
> +#ifdef __cplusplus

Get rid of this, this header's only useful in C++.  (The same in Util.h dates back a really long time, to when JSAPI was C.)

@@ +160,5 @@
> +};
> +
> +} // namespace mozilla
> +#endif /* __cplusplus */
> +#endif /* mozilla_Maybe_h */

A little breathing space:

} // namespace mozilla

#endif /* mozilla_Maybe_h */

Comment 4

[Justin Lebar (not reading bugmail)]

> ?  (although I'm not 100% certain Maybe is only used for stacked stuff, in practice -- 
> probably this is something we should clarify with MOZ_STACK_CLASS if so)

It's used inside a bunch of apparently non-stack classes.  Without any deleterious effects, afaict.

> +/* XXX */

Sorry I missed that!

Comment 5

[Justin Lebar (not reading bugmail)]

I went with

> /* A class for lazily constructing an object without sticking it on the heap. */

Comment 6

[Justin Lebar (not reading bugmail)]

Let's make sure this builds everywhere: https://tbpl.mozilla.org/?tree=Try&rev=39c76321b6f1

Comment 7

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/535e7c112fc6

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/535e7c112fc6

Comment 9

[Nicholas Nethercote [inactive]]

Fun fact:  adding jsapi.h to BindingDeclarations increased the number of built files that depend on jsapi.h from 1563 to 2157.  It appears to be entirely unnecessary.  I'll land a follow-up to remove it once I confirm that it builds everywhere.

Comment 10

[Justin Lebar (not reading bugmail)]

> Fun fact:  adding jsapi.h to BindingDeclarations increased the number of built files 
> that depend on jsapi.h from 1563 to 2157.

Okay, but I didn't do that in this patch!

Comment 11

[Nicholas Nethercote [inactive]]

> Okay, but I didn't do that in this patch!

You sure about that?

https://hg.mozilla.org/integration/mozilla-inbound/annotate/535e7c112fc6/dom/bindings/BindingDeclarations.h

Comment 12

[Nicholas Nethercote [inactive]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fc2901f48146

Comment 13

[Justin Lebar (not reading bugmail)]

Huh.  It was in my original patch as context, but you're right, I added it back.  I must have screwed up the merge.

Sorry about that!

Comment 14

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/fc2901f48146