Closed
Bug 1192130
Opened 9 years ago
Closed 9 years ago
Add MOZ_NON_AUTOABLE to restrict using auto in place of specific types
Categories
(Developer Infrastructure :: Source Code Analysis, defect)
Developer Infrastructure
Source Code Analysis
Tracking
(firefox43 fixed)
RESOLVED
FIXED
mozilla43
Tracking | Status | |
---|---|---|
firefox43 | --- | fixed |
People
(Reporter: ehsan.akhgari, Assigned: nika)
Details
Attachments
(2 files, 2 obsolete files)
5.34 KB,
patch
|
ehsan.akhgari
:
review+
|
Details | Diff | Splinter Review |
7.65 KB,
patch
|
Details | Diff | Splinter Review |
The following pattern is a memory leak since already_AddRef'ed destructor doesn't drop the reference: already_AddRefed<T> Foo(); auto obj = Foo(); We should add a MOZ_NON_AUTOABLE attribute which would apply to already_AddRefed and similar, so that we can detect these cases and convert them into compile time errors.
Assignee | ||
Comment 1•9 years ago
|
||
(In reply to Ehsan Akhgari (not reviewing patches, not reading bugmail, needinfo? me!) from comment #0) > The following pattern is a memory leak since already_AddRef'ed destructor > doesn't drop the reference: > > already_AddRefed<T> Foo(); > > auto obj = Foo(); > > We should add a MOZ_NON_AUTOABLE attribute which would apply to > already_AddRefed and similar, so that we can detect these cases and convert > them into compile time errors. This should be pretty trivial to implement, I'll get something working tomorrow hopefully.
Assignee: nobody → michael
Assignee | ||
Comment 2•9 years ago
|
||
Attachment #8646950 -
Flags: review?(ehsan)
Assignee | ||
Comment 3•9 years ago
|
||
Attachment #8646951 -
Flags: review?(ehsan)
Assignee | ||
Comment 4•9 years ago
|
||
Oops - I didn't include code for `auto &` and `const auto &` among others :S. try: https://treeherder.mozilla.org/#/jobs?repo=try&revision=2c3a3a8fe7eb
Attachment #8646950 -
Attachment is obsolete: true
Attachment #8646950 -
Flags: review?(ehsan)
Attachment #8646960 -
Flags: review?(ehsan)
Reporter | ||
Comment 5•9 years ago
|
||
Comment on attachment 8646960 [details] [diff] [review] Part 1: Add MOZ_NON_AUTOABLE to restrict using auto in place of certain types Review of attachment 8646960 [details] [diff] [review]: ----------------------------------------------------------------- r=me with the below fixed. ::: build/clang-plugin/clang-plugin.cpp @@ +1367,5 @@ > + const VarDecl *D = Result.Nodes.getNodeAs<VarDecl>("node"); > + > + if (const AutoType *T = D->getType()->getContainedAutoType()) { > + if (const CXXRecordDecl *Rec = T->getAsCXXRecordDecl()) { > + if (MozChecker::hasCustomAnnotation(Rec, "moz_non_autoable")) { Couldn't you use autoType() and hasDeducedType() AST matchers instead of doing the checks programatically? If not, this is fine too. ::: build/clang-plugin/tests/TestNoAutoType.cpp @@ +1,3 @@ > +#define MOZ_NON_AUTOABLE __attribute__((annotate("moz_non_autoable"))) > + > +struct MOZ_NON_AUTOABLE ExplicitType {}; Please also add a test where this annotation is applied to a template class. @@ +6,5 @@ > +void f() { > + ExplicitType a; > + auto b = a; // expected-error {{Cannot use auto to declare a variable of type 'ExplicitType'}} expected-note {{Please write out this type explicitly}} > + auto &br = a; // expected-error {{Cannot use auto to declare a variable of type 'ExplicitType'}} expected-note {{Please write out this type explicitly}} > + const auto &brc = a; // expected-error {{Cannot use auto to declare a variable of type 'ExplicitType'}} expected-note {{Please write out this type explicitly}} What about auto* and const auto*?
Attachment #8646960 -
Flags: review?(ehsan) → review+
Reporter | ||
Updated•9 years ago
|
Attachment #8646951 -
Flags: review?(ehsan) → review+
Assignee | ||
Comment 6•9 years ago
|
||
(In reply to Ehsan Akhgari (don't ask for review please) from comment #5) > Couldn't you use autoType() and hasDeducedType() AST matchers instead of > doing the checks programatically? If not, this is fine too. I could do the match autoType(hasDeducedType(nonAutoableType())), but that would unfortunately not give me enough information to be able to give good line numbers (as I won't have the VarDecl). If I decided to include the vardecl, unfortunately varDecl(hasType(autoType(..))) isn't the same as getContainedAutoType, as it doesn't strip away the & and const qualifiers, so I would have to separately match against each of those :-/. I think it makes the most sense to just do these checks programmatically. I think that this problem is a bit awkward to shoehorn into the matcher system, even though it is possible. If you think it's worth it, I can move it into the matcher system though.
Assignee | ||
Comment 7•9 years ago
|
||
(In reply to Michael Layzell [:mystor] from comment #6) > (In reply to Ehsan Akhgari (don't ask for review please) from comment #5) > > Couldn't you use autoType() and hasDeducedType() AST matchers instead of > > doing the checks programatically? If not, this is fine too. > > I could do the match > autoType(hasDeducedType(nonAutoableType())), but that would unfortunately > not give me enough information to be able to give good line numbers (as I > won't have the VarDecl). > > If I decided to include the vardecl, unfortunately > varDecl(hasType(autoType(..))) isn't the same as getContainedAutoType, as it > doesn't strip away the & and const qualifiers, so I would have to separately > match against each of those :-/. > > I think it makes the most sense to just do these checks programmatically. I > think that this problem is a bit awkward to shoehorn into the matcher > system, even though it is possible. If you think it's worth it, I can move > it into the matcher system though. Actually, I think I'll just add an `autoNonAutoableType()` matcher which uses getContainedAutoType internally, and performs the check there, that way I can move the logic into the matchers. Is that OK with you Ehsan?
https://hg.mozilla.org/integration/mozilla-inbound/rev/4a4eba03ae3e https://hg.mozilla.org/integration/mozilla-inbound/rev/0f595614207f
Comment 10•9 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/4a4eba03ae3e https://hg.mozilla.org/mozilla-central/rev/0f595614207f
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox43:
--- → fixed
Flags: in-testsuite+
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
•