Refactor the abstract class CreateConnectedElementI #1047
Conversation
…riant In the current implementation of the CPACS Creator, the Wing and Fuselage class are inherited from an abstract class. We want to avoid this behaviour and fix the code style by removing this class and adding a new std::variant type instead. In the actual code, it is then decided whether the current object is of type Wing or Fuselage and the respective member function is called.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## cpacs_creator_dev_merge #1047 +/- ##
========================================================
Coverage 71.40% 71.40%
========================================================
Files 312 312
Lines 28978 28976 -2
========================================================
Hits 20691 20691
+ Misses 8287 8285 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
|
| createConnectedElementI->CreateNewConnectedElementBefore(startUID); | ||
| // Construction to 'unwrap' the std::variant createConnectedElement and apply the defined function from the correct class (which is part of the std::variant) | ||
| std::visit( | ||
| [startUID](auto& element){ |
There was a problem hiding this comment.
You could capture startUID by reference here as well.
| [startUID](auto& element){ | |
| [&startUID](auto& element){ |
| createConnectedElementI->CreateNewConnectedElementAfter(startUID); | ||
| // Construction to 'unwrap' the std::variant createConnectedElement and apply the defined function from the correct class (which is part of the std::variant) | ||
| std::visit( | ||
| [startUID](auto& element){ |
There was a problem hiding this comment.
You could capture startUID by reference here as well
| [startUID](auto& element){ | |
| [&startUID](auto& element){ |
| createConnectedElementI->DeleteConnectedElement(uid.toStdString()); | ||
| // Construction to 'unwrap' the std::variant createConnectedElement and apply the defined function from the correct class (which is part of the std::variant) | ||
| std::visit( | ||
| [uid](auto& element){ |
There was a problem hiding this comment.
You could capture uid by reference here as well.
| elementI = dynamic_cast<tigl::CreateConnectedElementI* >(&fuselage); | ||
| Ui::ElementModificatorInterface* element; | ||
| try { | ||
| element = reinterpret_cast<Ui::ElementModificatorInterface* >(typePtr.ptr); |
There was a problem hiding this comment.
I am not entirely sure that this is safe. The UIDManager first casts a a CCPACSWing* or a CCPACSFuselage* to a void*. Then you reinterpret_cast this void pointer to a pointer of type ui::ElementModificatorInterface*. This might work, depending on memeroy layout, I really don't know to be honest. It might also result in undefined behavior.
Just to be save, we could go two ways:
- Redefine the
ui::ElementModificatorInterfaceto be a std:;variant<CCPACSWing*, CCPACSFuselage*>` and create an instance (not a pointer) directly from the wing or fuselage pointer - Don't use
std::variantat all. Instead, everywhere where you usestd::visitcurrently, you could use theTypedPtrand query the type in anif-then-else.
Currently, I am not sure which I prefer, both have their pros and cons....
Description
In the current implementation of the CPACS Creator, the Wing and Fuselage class are inherited from an abstract class (cf. #1042). We want to avoid this behaviour and fix the code style by removing this class and adding a new
std::varianttype instead. In the actual code, it is then decided whether the current object is of type Wing or Fuselage and the respective member function is called.Later on, the variant type can be extended by any other class (such as Duct, Pylon, Tank, ...), if the CPACS Creator should be able to add CPACS elements/sections within the according object.
The general procedure (and whether this is the best way) of course still can be discussed. Nonetheless, @joergbrech and I had a disscussion about it and agreed that using
std::variantseems to be a good way to solve this code style issue.Fix #1042