Closed
Bug 1285918
Opened 8 years ago
Closed 8 years ago
Analysis to produce an error when having operator new overloaded by a class with macro like NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW
Categories
(Developer Infrastructure :: Source Code Analysis, defect)
Developer Infrastructure
Source Code Analysis
Tracking
(firefox50 affected)
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
firefox50 | --- | affected |
People
(Reporter: andi, Assigned: andi)
References
Details
Attachments
(1 file)
58 bytes,
text/x-review-board-request
|
Details |
As we've discussed at our London All Hands Meeting we shouldn't allow in our code the overload operator new in different classes, so we must have a checker in our static analysis tool that checks this and if asserts true it should return a compile error.
Assignee | ||
Updated•8 years ago
|
Summary: Analysis to produce an error when having operator new overloaded by a class → Analysis to produce an error when having operator new overloaded by a class with macro NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW
Assignee | ||
Comment 1•8 years ago
|
||
To be more specific, this should be true only a default implementation for new operator is present like in case of macro: NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW
Assignee | ||
Updated•8 years ago
|
Summary: Analysis to produce an error when having operator new overloaded by a class with macro NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW → Analysis to produce an error when having operator new overloaded by a class with macro like NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW
Assignee | ||
Comment 2•8 years ago
|
||
What we want to avoid with this checker is the usage of declarations like:
>> void* operator new(size_t sz) CPP_THROW_NEW {
>> void* rv = ::operator new(sz);
>> if (rv) {
>> memset(rv, 0, sz);
>> }
>> return rv;
>> }
This kind of overloading of operator new shouldn't be allowed to exist in our code.
Assignee | ||
Comment 3•8 years ago
|
||
Review commit: https://reviewboard.mozilla.org/r/63684/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/63684/
Assignee | ||
Updated•8 years ago
|
Attachment #8770110 -
Flags: feedback?(amarchesini)
Comment 4•8 years ago
|
||
https://reviewboard.mozilla.org/r/63684/#review61210
Lgtm. But, is this what we decided during the meeting? If I remember correctly, the idea was to get rid of |operation new()| in our code base for member initialization.
We can use part of this code to avoid the use of such operator. And in the meantime we can remove macro: NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW
Updated•8 years ago
|
Attachment #8770110 -
Flags: feedback?(amarchesini)
Assignee | ||
Comment 5•8 years ago
|
||
what do you think Ehsan, should we have a checker like this one? I know that is we eliminate macro NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW this would become a false problem but still we can prevent this from ever happening.
Flags: needinfo?(ehsan)
Comment 6•8 years ago
|
||
Apologies for the long delay guys, I've been on vacation/sick after London...
I find it hard to justify banning this construct altogether, since it may have legitimate use cases that we're not currently aware of. I think for the purpose of member initialization, removing NS_DECL_AND_IMPL_ZEROING_OPERATOR_NEW and replacing its usages with normal initialization should be all that we need to do.
Status: NEW → RESOLVED
Closed: 8 years ago
Flags: needinfo?(ehsan)
Resolution: --- → WONTFIX
Updated•7 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
•