Conversation
There was a problem hiding this comment.
Pull request overview
This pull request adds clone methods to seven option implementation classes across the thermo and kinetics modules to enable deep copying of configuration objects.
Changes:
- Added clone() methods to base classes: NucleationOptionsImpl, ArrheniusOptionsImpl, and RateConstantOptionsImpl
- Added clone() methods to derived classes: EvaporationOptionsImpl and CoagulationOptionsImpl
- Added clone() methods to container classes: ThermoOptionsImpl and KineticsOptionsImpl that recursively clone nested option objects
Reviewed changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| src/thermo/thermo.hpp | Adds clone method to ThermoOptionsImpl that performs deep copy including nested nucleation options |
| src/thermo/nucleation.hpp | Adds clone method to NucleationOptionsImpl base class using copy constructor |
| src/kinetics/rate_constant.hpp | Adds clone method to RateConstantOptionsImpl using copy constructor |
| src/kinetics/kinetics.hpp | Adds clone method to KineticsOptionsImpl that performs deep copy including nested arrhenius, coagulation, and evaporation options |
| src/kinetics/evaporation.hpp | Adds clone method to EvaporationOptionsImpl derived class |
| src/kinetics/coagulation.hpp | Adds clone method to CoagulationOptionsImpl derived class |
| src/kinetics/arrhenius.hpp | Adds clone method to ArrheniusOptionsImpl base class using copy constructor |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| EvaporationOptionsImpl(const NucleationOptionsImpl& nucleation) | ||
| : NucleationOptionsImpl(nucleation) {} | ||
|
|
||
| std::shared_ptr<EvaporationOptionsImpl> clone() const { |
There was a problem hiding this comment.
The clone method should override the virtual clone method from the base class and be marked with 'override'. This ensures proper polymorphic behavior and makes the inheritance relationship explicit. Without virtual clone in the base class and override here, cloning through a base class pointer will cause object slicing.
| std::shared_ptr<EvaporationOptionsImpl> clone() const { | |
| std::shared_ptr<NucleationOptionsImpl> clone() const override { |
| virtual std::string name() const { return "arrhenius"; } | ||
| virtual ~ArrheniusOptionsImpl() = default; | ||
|
|
||
| std::shared_ptr<ArrheniusOptionsImpl> clone() const { |
There was a problem hiding this comment.
The clone method should be virtual to support polymorphic cloning. Since ArrheniusOptionsImpl is a polymorphic base class (it has virtual methods like name() and a virtual destructor) and has derived classes like CoagulationOptionsImpl, calling clone() through a base class pointer will cause object slicing if clone() is not virtual.
| std::shared_ptr<ArrheniusOptionsImpl> clone() const { | |
| virtual std::shared_ptr<ArrheniusOptionsImpl> clone() const { |
| CoagulationOptionsImpl(const ArrheniusOptionsImpl& arrhenius) | ||
| : ArrheniusOptionsImpl(arrhenius) {} | ||
|
|
||
| std::shared_ptr<CoagulationOptionsImpl> clone() const { |
There was a problem hiding this comment.
The clone method should override the virtual clone method from the base class and be marked with 'override'. This ensures proper polymorphic behavior and makes the inheritance relationship explicit. Without virtual clone in the base class and override here, cloning through a base class pointer will cause object slicing.
| std::shared_ptr<CoagulationOptionsImpl> clone() const { | |
| std::shared_ptr<CoagulationOptionsImpl> clone() const override { |
| std::shared_ptr<ThermoOptionsImpl> clone() const { | ||
| auto op = std::make_shared<ThermoOptionsImpl>(*this); | ||
| if (nucleation() != nullptr) { | ||
| op->nucleation() = nucleation()->clone(); | ||
| } | ||
| return op; | ||
| } |
There was a problem hiding this comment.
The new clone method lacks test coverage. Given that this is a critical functionality that involves deep copying of nested objects and the codebase has comprehensive test coverage for other features (as seen in the tests directory), tests should be added to verify that cloning works correctly, especially when nucleation contains derived class instances like EvaporationOptionsImpl.
| std::shared_ptr<KineticsOptionsImpl> clone() const { | ||
| auto op = std::make_shared<KineticsOptionsImpl>(*this); | ||
| if (arrhenius()) op->arrhenius() = arrhenius()->clone(); | ||
| if (coagulation()) op->coagulation() = coagulation()->clone(); | ||
| if (evaporation()) op->evaporation() = evaporation()->clone(); | ||
| return op; | ||
| } |
There was a problem hiding this comment.
The new clone method lacks test coverage. Given that this is a critical functionality that involves deep copying of nested objects (arrhenius, coagulation, evaporation) and the codebase has comprehensive test coverage for other features, tests should be added to verify that cloning works correctly, especially when these fields contain derived class instances.
| virtual std::string name() const { return "nucleation"; } | ||
| virtual ~NucleationOptionsImpl() = default; | ||
|
|
||
| std::shared_ptr<NucleationOptionsImpl> clone() const { |
There was a problem hiding this comment.
The clone method should be virtual to support polymorphic cloning. Since NucleationOptionsImpl is a polymorphic base class (it has virtual methods like name() and a virtual destructor) and has derived classes like EvaporationOptionsImpl, calling clone() through a base class pointer will cause object slicing if clone() is not virtual. This means that if a NucleationOptions pointer actually points to an EvaporationOptionsImpl object, calling clone() will only copy the NucleationOptionsImpl portion, losing all EvaporationOptionsImpl-specific data.
| std::shared_ptr<NucleationOptionsImpl> clone() const { | |
| virtual std::shared_ptr<NucleationOptionsImpl> clone() const { |
1 participant
No description provided.