Open artificiel opened 2 months ago
I think it has to do with how you can have two different instances of an ofParameter referencing the same value and that is why it is name as such. I think it has to do more about the purpose of it rather than the C++ correctness. But, it is a virtual abstract function so it needs to be defined in each class that extends ofAbstractParameter. I feel that this is more of a cosmetic change rather than an important one.
well, i'd argue it's more than cosmetic, as someone tackling the code has to deconstruct the idea that a method called newReference() has nothing to do with actual references (nor new
, to that effect), but with shared_ptr... and as i mentionned, the concept of a reference can be implemented through *, std::shared_ptr, &, and others, but the specific "reference" term means &
in C++. because the method is essentially internal, clarifying the interface is a gain with low risk (and anyway as proposed in code, nothing is removed, merely deprecated).
as a side note i'd like to reiter this came up in discussion with a mid-level workshop, in which one of the activities was to "drill into" OF functions, to build familiarity in the operation of looking at implementations to see how things actually work (notably when documentation is not optimal), and although minor the discrepancy was noted as unecessarily confusing.
I see your point. So, with this change you are doing is it spitting out any deprecation warnings? as you mention the sparse inline documentation, why not use this instance to add a bit to it? :)
I can produce docs if I know they will get merged (pending review of course, but not forgotten in a never-merged PR, or so late that they evolve conflicts with commits in the meantime).
right now there are a couple of PR's related to ofParameter in the queue; it would be better to merge what it standing by, and then making a clean docs pass.
recent:
older (too late?):
Let me check those and I will get back, but I guess that documentation will not interfere and probably an automerge will be allowed
@roymacdonald did you get a chance to look at the above listed PR's?
after a good discussion in a workshop on the subtleties of references vs pointers (and shared_ptr) it became evident that the
ofParameter::newReference()
method is ill-named, as it does not return a &reference but a shared_ptr.this deprecates and uses a more correct
getSharedPtr()
name for the same function. it does not seem very widely used — one even wonders why not simply usestd::make_shared()
at the call site...(perhaps the method above used the term "reference" in a more philosophical way, as yes a pointer "refers" to an object, but in C++ "reference" is very specific term that should not be used unless talking about actual &references, even more so in an API)