Closed Bug 1277896 Opened 3 years ago Closed 3 years ago

remove 'final' from Vector

Categories

(Core :: MFBT, defect)

defect
Not set

Tracking

()

RESOLVED WONTFIX
mozilla50
Tracking Status
firefox49 --- affected
firefox50 --- fixed

People

(Reporter: luke, Assigned: luke)

References

Details

Attachments

(1 file)

Attached patch rm-final-vectorSplinter Review
I need to create a new class adding a few domain-specific members to Vector, but I'd like to mostly forget about this class and use it as a Vector.  There's no risk of anyone deleting this thing via the non-virtual base-class, and I'd like avoid containment and adding some '.vector()' I have to call everywhere or some operator overload that will break down in various cases.  So I'd like to just derive Vector and add these members, but it's currently marked as 'final'.
Attachment #8759726 - Flags: review?(jwalden+bmo)
Blocks: 1276028
Waldo requested |final| in bug 1170325 comment 6.  I don't fully understand his reasoning here, since we don't have a lot of functions that take exactly Vector (appendAll is templated over Vector types; swap and move construct/assign take exactly Vector, but you wouldn't want them to take anything else; I don't think we have others...but maybe he was talking about functions outside of  Vector?).
https://hg.mozilla.org/mozilla-central/rev/03765412b2a8
Status: ASSIGNED → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla50
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Comment on attachment 8759726 [details] [diff] [review]
rm-final-vector

Review of attachment 8759726 [details] [diff] [review]:
-----------------------------------------------------------------

Encapsulation is better than inheritance for data structure-like classes in general, because it means the encapsulator class is responsible for maintaining the invariants of the encapsulated class.  Exposing the encapsulated class via inheritance, means every user of the subclass has to be careful to maintain the invariants.  It gets messy when we start talking about overriding functions in the "base" class, because it can be non-obvious what functions need to be overridden, and it's not going to be clear all the right ones have been overridden, or what functions *can* be safely exposed.

And, with public inheritance, people can accidentally slice away the base-class versions of superclass functions, e.g. by passing a SubclassVector& where a Vector& is expected and then getting the Vector versions of things, not the desirably-overridden SubclassVector versions of them.  For SubclassVector to provide extra sense to Vector, it *must* intermediate everything, and doing that correctly means encapsulation.

In contrast, having the encapsulator intermediate access explicitly, makes clear exactly what interactions are possible with the data structure.  It forces those interactions to happen, and it makes it impossible to avoid them, intentionally or by accident.  It is not that onerous to type out a handful of forwarding functions as a largely one-time cost (and maybe once per extra mode of interaction to forward, but such will be rare), not for the clarity and safety it provides.
Attachment #8759726 - Flags: review?(jwalden+bmo) → review-
And for what it's worth, appendAll was the original motivator in the older bug's comment, as far as matching types goes.
Status: REOPENED → RESOLVED
Closed: 3 years ago3 years ago
Resolution: --- → WONTFIX
Agreed on the the reasons for preferring containment over inheritance; no need to rehash that; it is, by now, well known.  However, putting 'final' on Vector makes that decision for 100% of users when there is no a priori reason for all those arguments to apply to every case.  I have one example cited here and "fixing" it to not use inheritance only added boilerplate (not just in the class definition but scattered around the uses too); the resulting code is simply (slightly) worse; no encapsulation was achieved (or appropriate).

Now, I'm all for using static types help catch common mistakes, but a critical ingredient is that, once you've seen the warning sign, you can say "no, I know what I'm doing".  Hence MOZ_IMPLICIT, *_cast, etc.  But there is no such escape hatch for final.  I'm also not arguing unilaterally against 'final'; I can imagine many situations where it'd be useful (and I've used it myself), but core utilities that are going to be used in 10,000 different contexts isn't one of them, imo.

In particular, the hazards you identify are not magically avoided by forcing containment:
 - If the containing class makes the field public or returns a mutable reference to the contained field, there is also no encapsulation.
 - If a pointer is taken to a contained field, it is at just as much risk as a "sliced" class for improper deletion.
So, as ever, there is no silver bullet; the programmer has to participate in the design of their code.

I also don't agree with this unilateral closing of the bug; I'm not certain you have that authority as this seems like a broader Mozilla coding convention question.  (I also wrote Vector, though I'm not going to attempt to appeal to that reason.)  But I'm not excited to waste any more of my time on arguing this ultimately work-around-able issue (at least until I hit another, perhaps more annoying, example), so for now I'll just note my objection, both to the decision and to the (lack of) process for reaching the decision.
You need to log in before you can comment on or make changes to this bug.