Expose only model description through execution #603
Conversation
|
Change looks fine to me. I double checked cosim4j, and the
No need (for cosim4j). I can do it, as the natives should be built and checked in for both windows and linux. |
include/cosim/observer/observer.hpp
Outdated
| @@ -26,10 +26,10 @@ class observable | |||
| { | |||
| public: | |||
| /// Returns the entity's name. | |||
| virtual std::string name() const = 0; | |||
| virtual const std::string& name() const = 0; | |||
There was a problem hiding this comment.
So this is something I've thought a lot about: whether, as a rule of thumb, virtual functions should return objects by value or reference.
If a function is non-virtual, so it is implemented in the class itself and not overridden by subclasses, it is common to return a reference if you can. After all, as the class implementer, you know whether there is some internal variable that you can return a reference to.
But if a function is virtual, and especially in an interface which is intended to be implemented by others, then it's not so clear anymore. Basically, you're forcing whoever implements your interface to have an internal buffer for the returned object in their class. This may be a hassle, especially if they wanted to generate the value on the fly or return a constant.
Was there a particular reason to switch to return-by-reference here?
There was a problem hiding this comment.
My sole motivation for doing this was to avoid unnecessary copies of said objects; I did not think about that it forces implementers to do things a certain way. I have no problem reverting this!
There was a problem hiding this comment.
I don't think any of these functions will be called in performance-critical situations, so I don't see the extra copies becoming a problem.
There was a problem hiding this comment.
And with C++11 if the value is generated in the function it might not even be a copy (https://en.cppreference.com/w/cpp/language/copy_elision)
This closes #602.
execution.get_simulator()has been replaced withexecution.get_model_description(). If this gets approved, I will create separate PR's for libcosimc and cosim4j updating the usage of the replaced method.I also did a change in
observer.hpp, returning const references for thename()andmodel_description()methods. Please let me know if that was a bad call!