Closed
Bug 1156802
Opened 9 years ago
Closed 9 years ago
Do not allow move constructors to be explicit
Categories
(Developer Infrastructure :: Source Code Analysis, defect)
Developer Infrastructure
Source Code Analysis
Tracking
(firefox40 affected, firefox43 fixed)
RESOLVED
FIXED
mozilla43
People
(Reporter: botond, Assigned: nika)
Details
Attachments
(2 files, 1 obsolete file)
6.92 KB,
patch
|
ehsan.akhgari
:
review+
froydnj
:
review+
|
Details | Diff | Splinter Review |
5.72 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
Is there a use case for making a move constructor explicit? If not, then - given that a move constructor being explicit has bitten us before (see bug 1156538) - perhaps we should enforce, via static analysis, that they *not* be explicit? (Suggested by :froydnj in bug 1156538 comment 5.)
Comment 1•9 years ago
|
||
I agree, we should force them to not be explicit.
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8656750 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•9 years ago
|
||
Attachment #8656752 -
Flags: review?(ehsan)
Assignee | ||
Comment 5•9 years ago
|
||
try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=ab2a3e472439
Comment 6•9 years ago
|
||
Comment on attachment 8656750 [details] [diff] [review] Part 1: Add an analysis which prohibits explicit move constructors Review of attachment 8656750 [details] [diff] [review]: ----------------------------------------------------------------- ::: build/clang-plugin/clang-plugin.cpp @@ +793,5 @@ > + > +// These two matchers are avaliable on more recent versions of clang, but are > +// not avaliable on all versions which we support, or on our testing > +// infrastructure. For that reason we prefix them with `moz`, so that the > +// name does not conflict if we are using an up-to-date build of clang Instead of doing this, let's add an isExplicitMoveCtor() matcher that combines both of these.
Attachment #8656750 -
Flags: review?(ehsan) → review-
Comment 7•9 years ago
|
||
Comment on attachment 8656752 [details] [diff] [review] Part 2: Remove all explicit move constructors Review of attachment 8656752 [details] [diff] [review]: ----------------------------------------------------------------- r=me on the dom parts, over to Nathan for XPCOM and MFBT.
Attachment #8656752 -
Flags: review?(nfroyd)
Attachment #8656752 -
Flags: review?(ehsan)
Attachment #8656752 -
Flags: review+
Assignee | ||
Comment 8•9 years ago
|
||
Attachment #8656750 -
Attachment is obsolete: true
Attachment #8657123 -
Flags: review?(ehsan)
Updated•9 years ago
|
Attachment #8657123 -
Flags: review?(ehsan) → review+
Updated•9 years ago
|
Attachment #8656752 -
Flags: review?(nfroyd) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/63f3a49b15cb https://hg.mozilla.org/integration/mozilla-inbound/rev/c1cdde5ccb2c
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/63f3a49b15cb https://hg.mozilla.org/mozilla-central/rev/c1cdde5ccb2c
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla43
Updated•6 years ago
|
Product: Core → Firefox Build System
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
•