Closed Bug 6217 Opened 26 years ago Closed 26 years ago

DOM generated code should use nsCOMPtr instead of NS_DEF_PTR

Categories

(Core :: DOM: Core & HTML, defect, P3)

defect

Tracking

()

RESOLVED FIXED

People

(Reporter: brendan, Assigned: scc-obsolete)

References

()

Details

See also http://lxr.mozilla.org/mozilla/search?string=NS_DEF_PTR A lot of the DOM files instantiate smart pointer classes, one per level 0 class, but never use the smart pointer type. What's more, scc@netscape.com found that the NS_DEF_PTR macro leads to a static destructor per class, per file of bloat, on Windows (at least, if the pointer type is used in the same file). Some of this stuff is literally useless; the rest vidur said he'd use nsCOMPtr instead, although if that's the case, do we need nsIPtr.h at all? Are there cases where we want a smart-pointer (smart enough to NS_IF_RELEASE on destruct) that's not an nsCOMPtr? /be
Status: NEW → ASSIGNED
Target Milestone: M7
Version: other
Summary: DOM generated code should use nsCOMPtr instead of NS_DEF_PTR
Will do this for M7. Since Peter Linss originally came up with the NS_DEF_PTR-based smart pointer work, it's possible that he still uses it in some of his code. If not, then I'd agree - we could probably get rid of it.
When we aggreed to let nsCOMPtr come online Scott said that he'd convert nsIPtr to be a wrapper for typedefing a nsCOMPtr. Apparently this didn't happen.
Assignee: vidur → scc
Status: ASSIGNED → NEW
Target Milestone: M7
I'm will momentarily check in a version of idlc that can generate code with nsCOMPtr instead of NS_DEF_PTR. I spent a bit of time analyzing the results and found the following with MSVC: when compiling both debug and optimized, the size of the object files and static libraries were a bit larger (marginally in an optimized, a bit more in debug) for the nsCOMPtr versions. It turns out that the unused NS_DEF_PTR defined classes are not compiled in (since they are declared in the scope of the source file and not used, the compiler decides that code doesn't need to be generated for them. Note that it's the compiler making the decision, not the linker - this is true for the object files). What I couldn't explain was the fact that the size of the DLL was identical for both the nsCOMPtr and NS_DEF_PTR cases (this was true for both debug and optimized). Since this didn't make sense, I had both Nisheeth and Rick Potts look over my shoulder while I created both versions. Scott, any thoughts on why that would be the case? Given the results, I haven't turned on nsCOMPtr code generation in the build. Scott, I'm reassigning this bug to you to see if you can help explain the results. To turn on nsCOMPtr code generation in the IDL compiler, do the following on Windows: 1) Uncomment the #define for USE_COMPTR in dom/tools/FileGen.h. 2) Make in the dom/tools directory. 3) Make in the dom/public/idl directory. 4) Make in the dom directory. Call me to discuss this if any of this explanation doesn't make sense.
Status: NEW → ASSIGNED
I'm looking at this. My first thought is that the smart pointers aren't used in very many places. My second thought is that where they are used, only the simplest facilities are exploited, where |nsCOMPtr| and the old macro-generated classes generated identical code, e.g., |operator->()|. I need to look at the right files, though, and will email Vidur to verify.
That's definitely true - smart pointers aren't used very much and are used in very simple ways. I've used the file dom/base/src/nsJSWindow.cpp as the basis for some of my analysis. It's one of the admittedly few files in the DOM project that use smart pointers. I had anticipated a large change in size because I had assumed that the unused NS_DEF_PTR declarations were generating code. Since I've found that they aren't, I understand that there shouldn't be a large change in code size. What's interesting is the larger nsCOMPtr code size and the identical DLL size. Given these, I'd like to know whether I should switch over to nsCOMPtrs or not.
We should get rid of the macro-generated smart-pointer classes because the mozilla world is simpler if we only have one smart-pointer implementation. If your use of smart-pointers is light enough that you are considering forgoing them all together, that is reasonable. If you want smart-pointers, I recommend | nsCOMPtr|. Keeping the macro classes around for the DOM will just make it too easy for other programmers to accidently choose the old scheme over the new. It's your call, and since it's local impact on your code is negligable, I can see your motivation. But the global impact of using |nsCOMPtr| within the DOM makes things that much simpler for people outside of the core Gecko team.
just waiting for Vidur's decision on how to move forward against this bug
Status: ASSIGNED → RESOLVED
Closed: 26 years ago
Resolution: --- → FIXED
Vidur tells me he has already or will immediately `throw the switch', and to mark this bug fixed.
Brendan, can you help us verify this bug? If you agree the resolution is correct please mark it as Verified. Thanks!
Mass update of qa contact
QA Contact: gerardok → janc
Component: DOM Level 1 → DOM HTML
QA contact Update
QA Contact: janc → desale
Updating QA contact to Shivakiran Tummala.
QA Contact: desale → stummala
Component: DOM: HTML → DOM: Core & HTML
QA Contact: stummala → general
You need to log in before you can comment on or make changes to this bug.