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.

Sign up for GitHub

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

Expose only model description through execution #603

Merged
merged 4 commits into from
Jul 1, 2020
Merged

Expose only model description through execution #603

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
23ddc11
#602 Replace get_simulator() with get_model_description()
eidekrist Jun 23, 2020
1ce83fd
Const ref observable.name() and observable.model_description()
eidekrist Jun 23, 2020
e652752
Revert "Const ref observable.name() and observable.model_description()"
eidekrist Jul 1, 2020
548bb5a
Don't return const reference to model_description object.
eidekrist Jul 1, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions include/cosim/execution.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -297,8 +297,8 @@ class execution
/// Returns the current real time factor target
double get_real_time_factor_target() const;

/// Returns the simulator with the given index
std::shared_ptr<const simulator> get_simulator(simulator_index index) const;
/// Returns the model description for a simulator with the given index
const model_description& get_model_description(simulator_index index) const;

/// Returns a map of currently modified variables
std::vector<variable_id> get_modified_variables() const;
Expand Down
4 changes: 2 additions & 2 deletions include/cosim/observer/observer.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)


/// Returns a description of the entity.
virtual cosim::model_description model_description() const = 0;
virtual const cosim::model_description& model_description() const = 0;

/**
* Exposes a variable for retrieval with `get_xxx()`.
Expand Down
9 changes: 4 additions & 5 deletions src/cosim/execution.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -192,9 +192,9 @@ class execution::impl
return timer_.get_real_time_factor_target();
}

std::shared_ptr<const simulator> get_simulator(simulator_index index) const
const model_description& get_model_description(simulator_index index) const
{
return simulators_.at(index);
return simulators_.at(index)->model_description();
}

std::vector<variable_id> get_modified_variables() const
Expand Down Expand Up @@ -444,10 +444,9 @@ double execution::get_real_time_factor_target() const
return pimpl_->get_real_time_factor_target();
}

std::shared_ptr<const simulator> execution::get_simulator(
simulator_index index) const
const model_description& execution::get_model_description(simulator_index index) const
{
return pimpl_->get_simulator(index);
return pimpl_->get_model_description(index);
}

std::vector<variable_id> execution::get_modified_variables() const
Expand Down
11 changes: 7 additions & 4 deletions src/cosim/observer/file_observer.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -164,7 +164,8 @@ class file_observer::slave_value_writer
void initialize_default()
{
if (!timeStampedFileNames_) {
const auto filePath = logDir_ / observable_->name().append(".csv");
auto name = observable_->name();
const auto filePath = logDir_ / name.append(".csv");
clear_file_contents_if_exists(filePath, fsw_);
}

Expand All @@ -179,7 +180,8 @@ class file_observer::slave_value_writer
void initialize_config(const std::vector<variable_description>& variables)
{
if (!timeStampedFileNames_) {
const auto filePath = logDir_ / observable_->name().append(".csv");
auto name = observable_->name();
const auto filePath = logDir_ / name.append(".csv");
clear_file_contents_if_exists(filePath, fsw_);
}

Expand All @@ -191,11 +193,12 @@ class file_observer::slave_value_writer
void create_log_file()
{
std::string filename;
auto name = observable_->name();
if (!timeStampedFileNames_) {
filename = observable_->name().append(".csv");
filename = name.append(".csv");
} else {
auto time_str = format_time(boost::posix_time::microsec_clock::local_time());
filename = observable_->name().append("_").append(time_str).append(".csv");
filename = name.append("_").append(time_str).append(".csv");
}

const auto filePath = logDir_ / filename;
Expand Down
8 changes: 4 additions & 4 deletions src/cosim/slave_simulator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -239,12 +239,12 @@ class slave_simulator::impl

impl& operator=(impl&&) noexcept = delete;

std::string name() const
const std::string& name() const
{
return name_;
}

cosim::model_description model_description() const
const cosim::model_description& model_description() const
{
return modelDescription_;
}
Expand Down Expand Up @@ -561,13 +561,13 @@ slave_simulator::slave_simulator(slave_simulator&&) noexcept = default;
slave_simulator& slave_simulator::operator=(slave_simulator&&) noexcept = default;


std::string slave_simulator::name() const
const std::string& slave_simulator::name() const
{
return pimpl_->name();
}


cosim::model_description slave_simulator::model_description() const
const cosim::model_description& slave_simulator::model_description() const
{
return pimpl_->model_description();
}
Expand Down
4 changes: 2 additions & 2 deletions src/cosim/slave_simulator.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -35,8 +35,8 @@ class slave_simulator : public simulator
slave_simulator& operator=(slave_simulator&&) noexcept;

// `observable` methods
std::string name() const override;
cosim::model_description model_description() const override;
const std::string& name() const override;
const cosim::model_description& model_description() const override;

void expose_for_getting(variable_type type, value_reference ref) override;
double get_real(value_reference reference) const override;
Expand Down