Closed Bug 909514 Opened 7 years ago Closed 7 years ago

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

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla26

People

(Reporter: justin.lebar+bug, Assigned: justin.lebar+bug)

References

Details

Attachments

(1 file, 1 obsolete file)

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.
Attached patch Patch, v1 (obsolete) — Splinter Review
Attachment #795654 - Flags: review?(jwalden+bmo)
Attached patch Patch, v2Splinter Review
Whoops, forgot to include the removal of Maybe from Util.h in the previous patch.
Attachment #795654 - Attachment is obsolete: true
Attachment #795654 - Flags: review?(jwalden+bmo)
Attachment #795670 - Flags: review?(jwalden+bmo)
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 */
Attachment #795670 - Flags: review?(jwalden+bmo) → review+
> ?  (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!
I went with

> /* A class for lazily constructing an object without sticking it on the heap. */
Let's make sure this builds everywhere: https://tbpl.mozilla.org/?tree=Try&rev=39c76321b6f1
https://hg.mozilla.org/mozilla-central/rev/535e7c112fc6
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla26
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.
> 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!
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!
You need to log in before you can comment on or make changes to this bug.