Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ofParameter: deprecate newReference() in favour of getSharedPtr() #8128

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

artificiel
Copy link
Contributor

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 use std::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)

@roymacdonald
Copy link
Member

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.

@artificiel
Copy link
Contributor Author

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.

@roymacdonald
Copy link
Member

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? :)

@artificiel
Copy link
Contributor Author

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:
#8128 (this)
#7900 #8127 #8132

older (too late?):
#7122 #6614

@roymacdonald
Copy link
Member

Let me check those and I will get back, but I guess that documentation will not interfere and probably an automerge will be allowed

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants