rmm icon indicating copy to clipboard operation
rmm copied to clipboard

[FEA] Consider making the lock types of memory resources private

Open vyasr opened this issue 2 years ago • 3 comments

Is your feature request related to a problem? Please describe. Classes like fixed_size_memory_resource define a lock type as a protected member. This exposes an implementation detail of the class that may not need to be visible to subclasses.

Describe the solution you'd like We should investigate making these attributes private instead of public. c.f. https://github.com/rapidsai/rmm/pull/1317#discussion_r1308197112

vyasr avatar Aug 30 '23 22:08 vyasr

@vyasr Do you still think these members should be private instead of protected? While I agree this feels like an implementation detail, it seems like it would be hard to customize the behavior of a subclass if you didn't have a way to access the locking mechanism of the parent class. I could go either way on this, just want to double-check that you think it's worthwhile before attempting any fixes here.

bdice avatar Jan 20 '25 21:01 bdice

I don't remember why, but most of our MR classes are final.

harrism avatar Jan 20 '25 22:01 harrism

This issue was originally opened because protected members need to be documented, as we discovered in #1317. That makes sense because if a class allows subclassing a consumer needs to know what methods they can safely override (although one could argue that there are edge cases for classes that should only be subclassed internally; in most such cases the parent class is probably internal-only and doesn't need to be documented in the public API at all, but that's not necessarily always true). If a class is declared as final then I don't think protected members make any sense since you can't subclass anyway and those members are effectively private. Therefore I think that the two reasonable options here are:

  1. Leave the classes as final and make the members private.
  2. Remove the final specifier and leave the members as protected. In that case, they should be documented (I should have already added all necessary documentation in #1317 so I don't think there's anything else to be done there).

I don't know why the classes are final. Whether or not there is a good reason for that may be sufficient to decide it. Maybe @jrhemstad remembers why?

vyasr avatar Jan 24 '25 20:01 vyasr