Closed Bug 1493276 Opened 3 years ago Closed 3 years ago
Tighten assertion in Call
46 bytes, text/x-phabricator-request
|Details | Review|
CallQueryInterface statically asserts that you don't QI an object with type A to type B when A is the same as B, but we should also ban it when A is a subclass of B (unless B is nsISupports), because a QI is slower than a static cast.
If a class A is derived from a class B, then an instance of A can be converted to an instance of class B via a static cast, so QI is not needed. QIs are slower than static casts. TestCallTemplates seems to be testing that CallQueryInterface compiles even if the first argument's class is only ambiguously castable to nsISupports, so I changed the second argument to be a class unrelated to the concrete class. I also removed some useless null checks on the return value of new.
Pushed by email@example.com: https://hg.mozilla.org/integration/autoland/rev/44e5267358eb Statically prevent CallQueryInterface to a base class r=froydnj
Jorg, this may or may not cause build failures in Thunderbird. They'll be static asserts at calls to CallQueryInterface, and you have to remove the CallQueryInterface and turn them into regular assignments as seen in my patch.
Comment on attachment 9011932 [details] Bug 1493276 - Statically prevent CallQueryInterface to a base class Nathan Froyd [:froydnj] has approved the revision.
Attachment #9011932 - Flags: review+
(In reply to Andrew McCreight [:mccr8] from comment #3) > Jorg, this may or may not cause build failures in Thunderbird. They'll be > static asserts at calls to CallQueryInterface, and you have to remove the > CallQueryInterface and turn them into regular assignments as seen in my > patch. Thanks for the warning. We have a few CallQueryInterface call sites, is there a way to tell which ones need replacement? So if they are build failures, I can apply the patch now, compile and see, right?
Yes, exactly. If it compiles, it's fine. If not, it needs fixing.
Yeah. And any fixes can land in advance of whenever my patch lands on comm-central.
I got at least two compile errors, so I'll file a bug and fix them, thanks again.
You need to log in before you can comment on or make changes to this bug.