Closed
Bug 1272203
Opened 8 years ago
Closed 8 years ago
Add mozilla::NotNull to MFBT
Categories
(Core :: MFBT, defect)
Core
MFBT
Tracking
()
RESOLVED
FIXED
mozilla49
Tracking | Status | |
---|---|---|
firefox49 | --- | fixed |
People
(Reporter: n.nethercote, Assigned: n.nethercote)
References
(Blocks 2 open bugs)
Details
Attachments
(4 files, 3 obsolete files)
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
froydnj
:
review+
|
Details | Diff | Splinter Review |
(deleted),
patch
|
n.nethercote
:
review+
|
Details | Diff | Splinter Review |
I have a draft implementation of mozilla::NotNull. It's similar to gsl::not_null, but with one major and one minor difference, which I will explain shortly.
![]() |
Assignee | |
Comment 1•8 years ago
|
||
This patch implements mozilla::NotNull, which is similar but not identicial to gsl::not_null. The current draft(?) implementation of gsl::not_null is at https://github.com/Microsoft/GSL/blob/master/include/gsl.h. The main difference is that not_null allows implicit conversion from T to not_null<T>. In contrast, NotNull only allows explicit conversion from T to NotNull<T> via WrapNotNull(). The rationale for this is that when converting from a less-constrained type to a more constrained type, implicit conversion is undesirable. For example, if I changed a function f from this: f(int* aPtr); to this: f(gsl::not_null<int*> aPtr); no call sites would have to be modified. But if I changed it to this: f(mozilla::NotNull<int*> aPtr); call sites *would* need to be modified. This is a good thing! It forces the author to audit the call sites for non-nullness, and encourages them to back-propagate NotNull throughout the code. The other difference between not_null and NotNull is that not_null disables pointer arithmetic, which means it cannot be used with array pointers. I have not implemented this restriction for NotNull because it seems arbitrary and unnecessary.
Attachment #8751588 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Updated•8 years ago
|
Assignee: nobody → n.nethercote
Status: NEW → ASSIGNED
![]() |
Assignee | |
Comment 3•8 years ago
|
||
This might not look compelling in isolation, but this use of NotNull would have prevented the null dereference crash in bug 1268721.
Attachment #8751590 -
Flags: review?(nfroyd)
![]() |
Assignee | |
Comment 4•8 years ago
|
||
This is a nice example of using NotNull for a single non-null pointer that gets passed around lots of different functions.
Attachment #8751591 -
Flags: review?(nfroyd)
Comment 5•8 years ago
|
||
Clang and gcc have `nonnull` and `returns_nonnull` attributes that may be interesting: http://clang.llvm.org/docs/AttributeReference.html#id102
![]() |
Assignee | |
Comment 6•8 years ago
|
||
(In reply to Chris Peterson [:cpeterson] from comment #5) > Clang and gcc have `nonnull` and `returns_nonnull` attributes that may be > interesting: > > http://clang.llvm.org/docs/AttributeReference.html#id102 Thank you for the info. These attributes appear to be very weak. E.g. if you have a _Nonnull parameter, clang will complain if you pass a literal |nullptr| to it, but won't complain if you pass a variable that is null. Also, they don't work with smart pointers like RefPtr. AFAICT NotNull<> is a much more robust mechanism.
Comment 7•8 years ago
|
||
Note also https://mxr.mozilla.org/mozilla-central/source/dom/bindings/BindingDeclarations.h?rev=c3f5e6079284#376 https://mxr.mozilla.org/mozilla-central/source/xpcom/base/OwningNonNull.h?rev=c3f5e6079284
![]() |
Assignee | |
Comment 8•8 years ago
|
||
(In reply to :Ms2ger from comment #7) > > https://mxr.mozilla.org/mozilla-central/source/dom/bindings/ > BindingDeclarations.h?rev=c3f5e6079284#376 > https://mxr.mozilla.org/mozilla-central/source/xpcom/base/OwningNonNull. > h?rev=c3f5e6079284 These are useful, but much less general than mozilla::NotNull. - dom::binding::NonNull doesn't work with smart pointers, and it allows vanilla assignment from a base pointer. - mozilla::OwningNonNull is a specialized version of RefPtr.
Comment 9•8 years ago
|
||
Comment on attachment 8751588 [details] [diff] [review] (part 1) - Add mozilla::NotNull to MFBT Review of attachment 8751588 [details] [diff] [review]: ----------------------------------------------------------------- ::: mfbt/NotNull.h @@ +147,5 @@ > +NotNull<T> > +WrapNotNull(const T aBasePtr) > +{ > + NotNull<T> notNull(aBasePtr); > + MOZ_RELEASE_ASSERT(aBasePtr); I'm not a fan of imposing overhead, even this minimal overhead, on people to use this. Just assert normally, and if people screw up, it's on their heads. Expect developers to be intelligent. @@ +153,5 @@ > +} > + > +// Compare two NotNulls. > +template <typename T, typename U> > +inline bool operator==(const NotNull<T>& aLhs, const NotNull<U>& aRhs) inline bool should be on its own line for all of these. @@ +165,5 @@ > +} > + > +// Compare a NotNull to a base pointer. > +template <typename T, typename U> > +inline bool operator==(const NotNull<T>& aLhs, const U& aRhs) Seems like you should also template <typename T> bool operator==(const NotNull<T>& aLhs, decltype(nullptr)) = delete; and vice versa to prevent those from compiling (because the answer's always vacuously true).
![]() |
Assignee | |
Comment 10•8 years ago
|
||
> ::: mfbt/NotNull.h
> @@ +147,5 @@
> > +NotNull<T>
> > +WrapNotNull(const T aBasePtr)
> > +{
> > + NotNull<T> notNull(aBasePtr);
> > + MOZ_RELEASE_ASSERT(aBasePtr);
>
> I'm not a fan of imposing overhead, even this minimal overhead, on people to
> use this. Just assert normally, and if people screw up, it's on their
> heads. Expect developers to be intelligent.
Two points:
- The compiler should be able to optimize away a lot of these assertions, because WrapNotNull() is inline and many WrapNotNull() calls will occur shortly after a null check.
- Several decades of experience has shown that "expect developers to be intelligent" is not a reliable strategy.
Comment 12•8 years ago
|
||
Will this implicitly convert from MakeUnique<T>? i.e. Will we be able to write: NotNull<UniquePtr<int>> k = MakeUnique<int>(5)? or do we have to write: NotNull<UniquePtr<int>> k = WrapNotNull(MakeUnique<int>(5))? The first is option is preferable to me.
Comment 13•8 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #12) I see that NotNull<UniquePtr<T>> doesn't work. I suppose the question could still stand for regular new.
Comment 14•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #10) > - The compiler should be able to optimize away a lot of these assertions But not all, and IMO this type *should* be the basis for optimization. > - Several decades of experience has shown that "expect developers to be > intelligent" is not a reliable strategy. No strategy is reliable. But the compiler/language already adopts the strategy of not nullchecking everything. Given that developers must go out of their way to use WrapNotNull, clearly demanding not-null meaning, this isolated demand for rigor isn't justified.
![]() |
Assignee | |
Comment 15•8 years ago
|
||
(In reply to Jeff Muizelaar [:jrmuizel] from comment #13) > I suppose the question could still stand for regular new. The current patch requires the use of MakeNotNull(). See comment 1 for an explanation why.
![]() |
||
Updated•8 years ago
|
Attachment #8751589 -
Flags: review?(nfroyd) → review+
![]() |
||
Comment 16•8 years ago
|
||
Comment on attachment 8751590 [details] [diff] [review] (part 3) - Use NotNull in nsContentUtils::GetSurfaceData() Review of attachment 8751590 [details] [diff] [review]: ----------------------------------------------------------------- ...or, at least, it would have forced people to think harder about whether something could have been non-null. But maybe not.
Attachment #8751590 -
Flags: review?(nfroyd) → review+
![]() |
||
Updated•8 years ago
|
Attachment #8751591 -
Flags: review?(nfroyd) → review+
![]() |
||
Comment 17•8 years ago
|
||
Comment on attachment 8751588 [details] [diff] [review] (part 1) - Add mozilla::NotNull to MFBT Review of attachment 8751588 [details] [diff] [review]: ----------------------------------------------------------------- I did not look too closely at the tests. f+ for getting some interface issues sorted out, but I'm inclined to accept this once we do. ::: mfbt/NotNull.h @@ +64,5 @@ > + > +#include "mozilla/Assertions.h" > + > +template <typename T> class nsAutoPtr; > +template <typename T> class RefPtr; What do we have these here for? @@ +97,5 @@ > +// > +// NotNull currently doesn't work with UniquePtr because UniquePtr<T> is not > +// copy-constructible, NotNull<T> is not move-constructible, so > +// NotNull<UniquePtr<T>> is neither copy- nor move-constructible. See > +// https://github.com/Microsoft/GSL/issues/89 for some more discussion. Perhaps for now we should: static_assert(IsPointer<T>::value, "only usable with raw pointer types!"); until we get enough template wizardry to support smart pointers efficiently (see also discussion on |get()| and |operator()| below)? @@ +108,5 @@ > + T mBasePtr; > + > + // This constructor is only used by WrapNotNull(). > + template <typename U> > + explicit NotNull(U aBasePtr) : mBasePtr(aBasePtr) {} I'm wondering if we want a U&& constructor here, so that one could say, e.g. in part 3: RefPtr<DataSourceSurface> maybeNullSurface = surface->GetDataSurface(); if (!dataSurface) { ... } NotNull<RefPtr<DataSourceSurface>> dataSurface = WrapNotNull(Move(maybeNullSurface)); @@ +120,5 @@ > + MOZ_IMPLICIT NotNull(const NotNull<U>& aOther) : mBasePtr(aOther.get()) {} > + > + // Default copy construction and assignment. > + NotNull(const NotNull<T>&) = default; > + NotNull<T>& operator=(const NotNull<T>&) = default; Can you explicitly add |= delete| for move construction/assignment? And FWIW, |NotNull| works just fine as a type name inside the class, no need to NotNull<T>. @@ +127,5 @@ > + // Nb: This has no effect when T is a raw pointer -- the implicit conversion > + // below applies in boolean context, alas. > + explicit operator bool() const = delete; > + > + // Explicit conversion to a base pointer. Use only to resolve ambiguity or to Saying "base pointer" when the code returns a reference seems confusing; I know what you mean, but I'm unsure of how to say it better. @@ +130,5 @@ > + > + // Explicit conversion to a base pointer. Use only to resolve ambiguity or to > + // get a castable pointer. > + const T& get() const { return mBasePtr; } > + T& get() { return mBasePtr; } I'm not excited about this or the non-const operator() below, since this opens up the ability to modify the internals of the class and thereby break invariants. Presumably this functionality is there for smart pointer types, so that .get() on NonNull<RefPtr<T>> doesn't do extra reference counting? Not a fan. @@ +134,5 @@ > + T& get() { return mBasePtr; } > + > + // Implicit conversion to a base pointer. Preferable to get(). > + operator const T&() const { return get(); } > + operator T&() { return get(); } This is partly here so that passing NotNull<T> to a function that takes T* just works without any extra effort? I think we'd eventually want to implement the suggestion from the above-referenced github issue that .get() on a not_null<smart_ptr<T>> returns smart_ptr::get(). @@ +147,5 @@ > +NotNull<T> > +WrapNotNull(const T aBasePtr) > +{ > + NotNull<T> notNull(aBasePtr); > + MOZ_RELEASE_ASSERT(aBasePtr); I sympathize with Waldo's position here. But I'm inclined to keep this in, because being able to pass a null pointer to WrapNotNull breaks the whole point of NotNull existing. If NotNull gets used, and we get crashes about NotNull things being null because we don't have this assert, then we're no better off than we were before. But if NotNull gets used and we keep the assert, then we get a crash at the point when things started to go sideways, not when they're already off the road and sliding through the forest somewhere. And that's much more useful. (Though I'm not sure how many of our null-pointer-or-close-to-it crashes are not immediately obvious what's going wrong...but then again, I know I've seen "well, just add a null pointer check" patches go by, and it seems like we could do better than that with something like this assert.) There is a small runtime cost and a less-small space cost (extra code, strings for filenames, assert messages, etc.). It's not obvious that all the uses of WrapNotNull will result in runtime checks, either; the compiler can probably optimize away some of them. It certainly can in some of Nick's later patches. For the small number of uses we're going to have initially, I don't think that's awful. Perhaps we can use MOZ_DIAGNOSTIC_ASSERT as a trial instead?
Attachment #8751588 -
Flags: review?(nfroyd) → feedback+
![]() |
Assignee | |
Comment 18•8 years ago
|
||
> > +template <typename T> class nsAutoPtr; > > +template <typename T> class RefPtr; > > What do we have these here for? Cruft from an earlier version, no longer needed. I'll remove. > @@ +97,5 @@ > > +// > > +// NotNull currently doesn't work with UniquePtr because UniquePtr<T> is not > > +// copy-constructible, NotNull<T> is not move-constructible, so > > +// NotNull<UniquePtr<T>> is neither copy- nor move-constructible. See > > +// https://github.com/Microsoft/GSL/issues/89 for some more discussion. > > Perhaps for now we should: > > static_assert(IsPointer<T>::value, "only usable with raw pointer types!"); > > until we get enough template wizardry to support smart pointers efficiently > (see also discussion on |get()| and |operator()| below)? If NotNull doesn't work with RefPtr and nsCOMPtr that takes away a big chunk of the motivation for using it. So I've left that unchanged for now. > > + template <typename U> > > + explicit NotNull(U aBasePtr) : mBasePtr(aBasePtr) {} > > I'm wondering if we want a U&& constructor here, so that one could say, e.g. > in part 3: > > RefPtr<DataSourceSurface> maybeNullSurface = surface->GetDataSurface(); > if (!dataSurface) { > ... > } > > NotNull<RefPtr<DataSourceSurface>> dataSurface = > WrapNotNull(Move(maybeNullSurface)); I haven't done that for now. > @@ +120,5 @@ > > + MOZ_IMPLICIT NotNull(const NotNull<U>& aOther) : mBasePtr(aOther.get()) {} > > + > > + // Default copy construction and assignment. > > + NotNull(const NotNull<T>&) = default; > > + NotNull<T>& operator=(const NotNull<T>&) = default; > > Can you explicitly add |= delete| for move construction/assignment? I tried adding those, but I got this error about the constructor: > In file included from Unified_cpp_widget0.cpp:101: > /home/njn/moz/mi9/widget/PuppetWidget.cpp:993:36: error: copying parameter of type 'mozilla::NotNull<mozilla::gfx::DataSourceSurface *>' invokes deleted constructor > nsContentUtils::GetSurfaceData(WrapNotNull(dataSurface), &length, &stride); > ^~~~~~~~~~~~~~~~~~~~~~~~ > ../dist/include/mozilla/NotNull.h:124:3: note: 'NotNull' has been explicitly marked deleted here > NotNull(NotNull<T>&&) = delete; > ^ and these errors about the assignment operator: > In file included from Unified_cpp_xpcom_threads0.cpp:119: > /home/njn/moz/mi9/xpcom/threads/nsThread.cpp:1181:13: error: overload resolution selected deleted operator '=' > mEvents = WrapNotNull(mEvents->mNext); > ~~~~~~~ ^ ~~~~~~~~~~~~~~~~~~~~~~~~~~~ > ../../dist/include/mozilla/NotNull.h:125:15: note: candidate function has been explicitly deleted > NotNull<T>& operator=(NotNull<T>&&) = delete; > ^ > ../../dist/include/mozilla/NotNull.h:121:15: note: candidate function > NotNull<T>& operator=(const NotNull<T>&) = default; > ^ > 1 error generated. I don't understand these errors. I've left that part unchanged for now. > And FWIW, |NotNull| works just fine as a type name inside the class, no need to > NotNull<T>. I've been using NotNull<T> because sometimes there is NotNull<U>, which does require the template parameter, and I like the consistency. I've left it unchanged for now, but I can remove if you feel strongly about it. > @@ +127,5 @@ > > + // Nb: This has no effect when T is a raw pointer -- the implicit conversion > > + // below applies in boolean context, alas. > > + explicit operator bool() const = delete; > > + > > + // Explicit conversion to a base pointer. Use only to resolve ambiguity or to > > Saying "base pointer" when the code returns a reference seems confusing; I > know what you mean, but I'm unsure of how to say it better. The comment at the top of the class defines "base pointer" as a raw or smart pointer, so I don't know how to improve this. > > + // Explicit conversion to a base pointer. Use only to resolve ambiguity or to > > + // get a castable pointer. > > + const T& get() const { return mBasePtr; } > > + T& get() { return mBasePtr; } > > I'm not excited about this or the non-const operator() below, since this > opens up the ability to modify the internals of the class and thereby break > invariants. Presumably this functionality is there for smart pointer types, > so that .get() on NonNull<RefPtr<T>> doesn't do extra reference counting? > Not a fan. I was able to remove the non-const get() and operator() and operator->. I also changed operator*'s return type to const. > I think we'd eventually want to implement the suggestion from the above-referenced github issue that .get() on a > not_null<smart_ptr<T>> returns smart_ptr::get(). I don't know how to implement that. Any tips? > Perhaps we can use MOZ_DIAGNOSTIC_ASSERT as a trial instead? That would be better than MOZ_ASSERT, but I still prefer MOZ_RELEASE_ASSERT. A few weeks of crash report triage is a bracing reminder of the value of disabled checks. Let's be more Rust-like where we can.
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8751588 -
Attachment is obsolete: true
![]() |
||
Comment 20•8 years ago
|
||
Comment on attachment 8755711 [details] [diff] [review] (part 1) - Add mozilla::NotNull to MFBT Review of attachment 8755711 [details] [diff] [review]: ----------------------------------------------------------------- Forgive the drive by... ::: mfbt/NotNull.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/. */ > + > +#ifndef mozilla_Ptr_h This file is called NotNull.h and is included as "mozilla/NotNull.h", so I believe the include guard should be "mozilla_NotNull_h" instead?
Comment 21•8 years ago
|
||
(In reply to Nicholas Nethercote [:njn] from comment #18) > > @@ +120,5 @@ > > > + MOZ_IMPLICIT NotNull(const NotNull<U>& aOther) : mBasePtr(aOther.get()) {} > > > + > > > + // Default copy construction and assignment. > > > + NotNull(const NotNull<T>&) = default; > > > + NotNull<T>& operator=(const NotNull<T>&) = default; > > > > Can you explicitly add |= delete| for move construction/assignment? > > I tried adding those, but I got this error about the constructor: > > > In file included from Unified_cpp_widget0.cpp:101: > > /home/njn/moz/mi9/widget/PuppetWidget.cpp:993:36: error: copying parameter of type 'mozilla::NotNull<mozilla::gfx::DataSourceSurface *>' invokes deleted constructor > > nsContentUtils::GetSurfaceData(WrapNotNull(dataSurface), &length, &stride); > > ^~~~~~~~~~~~~~~~~~~~~~~~ > > ../dist/include/mozilla/NotNull.h:124:3: note: 'NotNull' has been explicitly marked deleted here > > NotNull(NotNull<T>&&) = delete; > > ^ |WrapNotNull(dataSurface)| is an rvalue, so overload resolution, which does not take into account the fact that a candidate is deleted, chooses the move constructor. What is the motivation for deleting the move constructor / assignment operator?
![]() |
||
Comment 22•8 years ago
|
||
(In reply to Botond Ballo [:botond] from comment #21) > |WrapNotNull(dataSurface)| is an rvalue, so overload resolution, which does > not take into account the fact that a candidate is deleted, chooses the move > constructor. > > What is the motivation for deleting the move constructor / assignment > operator? I was asking for it to be deleted, but I should have asked for |= default| if that was reasonable, or for it to be explicitly defined. My fault for wasting time there.
![]() |
Assignee | |
Comment 23•8 years ago
|
||
(In reply to :Cykesiopka from comment #20) > > > > +#ifndef mozilla_Ptr_h > > This file is called NotNull.h and is included as "mozilla/NotNull.h", so I > believe the include guard should be "mozilla_NotNull_h" instead? Good catch. Thank you. > I was asking for it to be deleted, but I should have asked for |= default| > if that was reasonable, or for it to be explicitly defined. I'll add default versions.
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8755711 -
Attachment is obsolete: true
Attachment #8755711 -
Flags: feedback?(nfroyd)
![]() |
||
Comment 25•8 years ago
|
||
Comment on attachment 8756167 [details] [diff] [review] (part 1) - Add mozilla::NotNull to MFBT Review of attachment 8756167 [details] [diff] [review]: ----------------------------------------------------------------- OK, let's try this. A couple of comments below. ::: mfbt/NotNull.h @@ +119,5 @@ > + // Default copy/move construction and assignment. > + NotNull(const NotNull<T>&) = default; > + NotNull<T>& operator=(const NotNull<T>&) = default; > + NotNull(NotNull<T>&&) = default; > + NotNull<T>& operator=(NotNull<T>&&) = default; This bit contradicts the comment above the class which claims that NotNull<T> is not move-constructable. I think that means the comment needs to be modified. @@ +129,5 @@ > + // get a castable pointer. > + const T& get() const { return mBasePtr; } > + > + // Implicit conversion to a base pointer. Preferable to get(). > + operator const T&() const { return get(); } On second thought, returning |const T&| here instead of |T&| is going to cause issues with code like: extern void f(T*); NotNull<T*> x = ...; f(x); You could secretly view that as encouragement to convert f to use NotNull parameters, but sometimes that might not be feasible (f might be a third-party library function, for instance, but you still want to track NotNull-ness in your own application code). Can you file a followup bug on having: // If T is a smart pointer type: const T& get() const; operator const T&() const; // If T is a raw pointer type: T get(); operator T*(); and I can try to attempt template wizardry? In the meantime, let's remove the |const| qualifiers from the return type and the method. @@ +145,5 @@ > + MOZ_RELEASE_ASSERT(aBasePtr); > + return notNull; > +} > + > +// Compare two NotNulls. Were you able to implement Waldo's suggestion of |= delete|'ing comparisons to nullptr? I don't know overload and template instantiation rules well enough to know whether a comparison to nullptr will fail, because you don't have an appropriate overload defined, or whether the (NotNull&, const U&) (with U = some pointer type) would get instantiated. I *think* the first?
Attachment #8756167 -
Flags: review?(nfroyd) → review+
![]() |
Assignee | |
Comment 26•8 years ago
|
||
> This bit contradicts the comment above the class which claims that > NotNull<T> is not move-constructable. I think that means the comment needs > to be modified. Ok, will do. > On second thought, returning |const T&| here instead of |T&| is going to > cause issues with code like: > > extern void f(T*); > > NotNull<T*> x = ...; > f(x); > > You could secretly view that as encouragement to convert f to use NotNull > parameters, but sometimes that might not be feasible (f might be a > third-party library function, for instance, but you still want to track > NotNull-ness in your own application code). I don't think there's a problem here. My unit test has lots of calls like that and they work ok. > Can you file a followup bug on having: > > // If T is a smart pointer type: > const T& get() const; > operator const T&() const; > > // If T is a raw pointer type: > T get(); > operator T*(); > > and I can try to attempt template wizardry? I can do it if you still think it's necessary. > Were you able to implement Waldo's suggestion of |= delete|'ing comparisons > to nullptr? I've done this. Explicitly delete'ing them was necessary to prevent such comparisons from compiling.
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8756167 -
Attachment is obsolete: true
![]() |
Assignee | |
Updated•8 years ago
|
Attachment #8757075 -
Flags: review+
![]() |
Assignee | |
Comment 28•8 years ago
|
||
I got a compile error I don't understand on try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=fdb85997225c The error is this: > In file included from /builds/slave/try-lx-00000000000000000000000/build/src/dom/base/nsContentUtils.h:35:0, > from /builds/slave/try-lx-00000000000000000000000/build/src/widget/PuppetWidget.cpp:22, > from /builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/widget/Unified_cpp_widget0.cpp:101: > /builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/dist/include/mozilla/NotNull.h: In instantiation of 'class mozilla::NotNull<RefPtr<mozilla::gfx::DataSourceSurface> >': > /builds/slave/try-lx-00000000000000000000000/build/src/widget/PuppetWidget.cpp:992:52: required from here > /builds/slave/try-lx-00000000000000000000000/build/src/obj-firefox/dist/include/mozilla/NotNull.h:137:29: error: 'const' qualifiers cannot be applied to 'mozilla::gfx::DataSourceSurface&' > const decltype(*mBasePtr) operator*() const { return *mBasePtr; } The error only occurred on GCC on Try. I couldn't reproduce it locally, even when I installed GCC-4.8.5. I ended up removing the |const| from the return type of |operator*| which fixed it. I'm still not sure if this is some kind of bug in whatever version of GCC we use on Try.
Comment 29•8 years ago
|
||
FWIW: While not the source for Ubuntu's 4.8.5, this tells Ubuntu's GCC is far from pristine, and differences may well be coming from there: http://bazaar.launchpad.net/~ubuntu-branches/ubuntu/wily/gcc-4.8/wily/files/head:/debian/patches/ In contrast, our GCC 4.8.5 is built with this script: https://dxr.mozilla.org/mozilla-central/rev/b0096c5c727749ad3e79cbdf20d2e96bd179c213/build/unix/build-gcc/build-gcc.sh which applies exactly one patch: https://dxr.mozilla.org/mozilla-central/source/build/unix/build-gcc/PR64905.patch for this issue: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=64905
![]() |
Assignee | |
Comment 30•8 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/19f6a028f736c65e4848d901d33d6bad68294ee6 Bug 1272203 (part 1) - Add mozilla::NotNull to MFBT. r=froydnj. https://hg.mozilla.org/integration/mozilla-inbound/rev/12a0df9bb7edefaee2cb9845f94757eb082b546a Bug 1272203 (part 2) - Use NotNull in nsThread. r=froydnj. https://hg.mozilla.org/integration/mozilla-inbound/rev/c329d2652ad52f3261db1cc8bdbe4d68a220ab19 Bug 1272203 (part 3) - Use NotNull in nsContentUtils::GetSurfaceData(). r=froydnj. https://hg.mozilla.org/integration/mozilla-inbound/rev/a797b1192ac89f6f11049e2ea6d87dd66fb1cedc Bug 1272203 (part 4) - Use NotNull for XPTCursor. r=froydnj.
Comment 31•8 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/19f6a028f736 https://hg.mozilla.org/mozilla-central/rev/12a0df9bb7ed https://hg.mozilla.org/mozilla-central/rev/c329d2652ad5 https://hg.mozilla.org/mozilla-central/rev/a797b1192ac8
Status: ASSIGNED → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla49
Depends on: 1410760
You need to log in
before you can comment on or make changes to this bug.
Description
•