Closed
Bug 1493093
Opened 6 years ago
Closed 6 years ago
Maybe<non-temporary>() should be allowed
Categories
(Developer Infrastructure :: Source Code Analysis, defect)
Developer Infrastructure
Source Code Analysis
Tracking
(firefox64 fixed)
RESOLVED
FIXED
mozilla64
Tracking | Status | |
---|---|---|
firefox64 | --- | fixed |
People
(Reporter: glandium, Assigned: glandium)
References
Details
Attachments
(1 file)
Another set of static analysis errors that show up on mac when upgrading to clang 7, that should have been caught by clang 6 but haven't is: In file included from /builds/worker/workspace/build/src/obj-firefox/toolkit/recordreplay/Unified_cpp_toolkit_recordreplay1.cpp:29: /builds/worker/workspace/build/src/toolkit/recordreplay/ipc/JSControl.cpp:441:28: error: variable of type 'dom::Optional<HandleObject>' (aka 'Optional<Handle<JSObject *> >') is not valid in a temporary dom::Optional<HandleObject>(), &obj, er); ^ /builds/worker/workspace/build/src/toolkit/recordreplay/ipc/JSControl.cpp:441:28: note: value incorrectly allocated in a temporary /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingDeclarations.h:234:7: note: 'dom::Optional<HandleObject>' (aka 'Optional<Handle<JSObject *> >') is a non-temporary type because it inherits from a non-temporary type 'Optional_base<JS::Handle<JSObject *>, JS::Rooted<JSObject *> >' class Optional<JS::Handle<T> > : ^ /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingDeclarations.h:217:23: note: 'Optional_base<JS::Handle<JSObject *>, JS::Rooted<JSObject *> >' is a non-temporary type because member 'mImpl' is a non-temporary type 'Maybe<JS::Rooted<JSObject *> >' Maybe<InternalType> mImpl; ^ /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Maybe.h:167:69: note: 'Maybe<JS::Rooted<JSObject *> >' is a non-temporary type because it has a template argument non-temporary type 'JS::Rooted<JSObject *>' class MOZ_NON_PARAM MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS Maybe ^ In file included from /builds/worker/workspace/build/src/obj-firefox/toolkit/recordreplay/Unified_cpp_toolkit_recordreplay1.cpp:47: /builds/worker/workspace/build/src/toolkit/recordreplay/ipc/ParentGraphics.cpp:105:28: error: variable of type 'dom::Optional<HandleObject>' (aka 'Optional<Handle<JSObject *> >') is not valid in a temporary dom::Optional<HandleObject>(), &obj, er); ^ /builds/worker/workspace/build/src/toolkit/recordreplay/ipc/ParentGraphics.cpp:105:28: note: value incorrectly allocated in a temporary /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingDeclarations.h:234:7: note: 'dom::Optional<HandleObject>' (aka 'Optional<Handle<JSObject *> >') is a non-temporary type because it inherits from a non-temporary type 'Optional_base<JS::Handle<JSObject *>, JS::Rooted<JSObject *> >' class Optional<JS::Handle<T> > : ^ /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/dom/BindingDeclarations.h:217:23: note: 'Optional_base<JS::Handle<JSObject *>, JS::Rooted<JSObject *> >' is a non-temporary type because member 'mImpl' is a non-temporary type 'Maybe<JS::Rooted<JSObject *> >' Maybe<InternalType> mImpl; ^ /builds/worker/workspace/build/src/obj-firefox/dist/include/mozilla/Maybe.h:167:69: note: 'Maybe<JS::Rooted<JSObject *> >' is a non-temporary type because it has a template argument non-temporary type 'JS::Rooted<JSObject *>' class MOZ_NON_PARAM MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS Maybe ^ That's not a false positive in strict terms, but it can be argued that it should be a negative. Because Maybe<Foo>() doesn't actually contain a Foo. Now, another interesting fact is that Maybe comes with a Nothing. So instead of Maybe<Foo>(), one can actually use Nothing(). But Optional doesn't have the same thing... but it could. I'll file a separate bug for that. Whether we still want to relax the static analysis can still be discussed here.
Assignee | ||
Comment 1•6 years ago
|
||
> So instead of Maybe<Foo>(), one can actually use Nothing(). But Optional doesn't have the same thing... but it could. I'll file a separate bug for that.
Well, dom::ChromeUtils::Import takes a Optional<...>&, so that wouldn't help anyways. There needs to be an Optional<> temporary, and that'd be rejected by static analysis.
Assignee | ||
Comment 2•6 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #1) > > So instead of Maybe<Foo>(), one can actually use Nothing(). But Optional doesn't have the same thing... but it could. I'll file a separate bug for that. > > Well, dom::ChromeUtils::Import takes a Optional<...>&, so that wouldn't help > anyways. There needs to be an Optional<> temporary, and that'd be rejected > by static analysis. although... Optional<JS::Handle<JSObject*>>&... where JS::Handle<T> contains a const T*... oh my... triple indirection.
Assignee | ||
Updated•6 years ago
|
Summary: Maybe<temporary>() should be allowed → Maybe<non-temporary>() should be allowed
Comment 3•6 years ago
|
||
As per our discussion on IRC, we should whitelist this case with an annotation. I see here two options: 1. expand the current matcher AstMatcher->addMatcher(materializeTemporaryExpr().bind("node"), this); and also make it match against CXXTemporaryObjectExpr. This will help us by avoiding the need to do the removal of ImplicitCast and since CXXTemporaryObjectExpr is direct decedent of CXXConstructorExpr, see [1] we will have direct access to annotation. 2. The second option will be to filter it out here [2], but doing so we will have to do a double isa<...> and I'm not very fund of that and also we need to explicitly remove the ImplicitCast. In conclusion I will stick with option 1 since it's the easier solution, the only downside of it is that we need to bind CXXConstructorExpr to a new node and check it here [1]. [1] https://clang.llvm.org/doxygen/classclang_1_1CXXTemporaryObjectExpr.html [2] https://dxr.mozilla.org/mozilla-central/source/build/clang-plugin/ScopeChecker.cpp#77
Summary: Maybe<non-temporary>() should be allowed → Maybe<temporary>() should be allowed
Updated•6 years ago
|
Summary: Maybe<temporary>() should be allowed → Maybe<non-temporary>() should be allowed
Assignee | ||
Comment 4•6 years ago
|
||
Comment 5•6 years ago
|
||
Comment on attachment 9011178 [details] Bug 1493093 - Allow to relax MOZ_NON_TEMPORARY_CLASS for some specific constructors Andi-Bogdan Postelnicu [:andi] has approved the revision.
Attachment #9011178 -
Flags: review+
Pushed by mh@glandium.org: https://hg.mozilla.org/integration/autoland/rev/5c8f8fe4ef3e Allow to relax MOZ_NON_TEMPORARY_CLASS for some specific constructors r=andi
Comment 7•6 years ago
|
||
bugherder |
https://hg.mozilla.org/mozilla-central/rev/5c8f8fe4ef3e
Status: NEW → RESOLVED
Closed: 6 years ago
status-firefox64:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
Updated•6 years ago
|
Assignee: nobody → mh+mozilla
Updated•2 years ago
|
Product: Firefox Build System → Developer Infrastructure
You need to log in
before you can comment on or make changes to this bug.
Description
•