API: Major refactoring of all codes to use a strict Interface/AbstractBaseClass, Concrete style of naming#1537
Conversation
5328ed3 to
284b797
Compare
Phase 15 Summary: Fix I-Prefix Naming ViolationsPhase 15 completes the COM-style naming convention cleanup. All Part A: Renamed 8 I-prefixed non-interfaces → Abstract*These classes used the
Part B: Promoted AbstractStringStore → IStringStore
Applied Across Repositories
Post-Rebase FixesAfter rebasing onto the latest develop branches, several conflicts were resolved:
|
284b797 to
b19af99
Compare
Complete Summary of Name Changes in This BranchThis branch enforces a strict COM-style naming convention across the entire codebase:
Classes Renamed (Old → New)Phase 2
Phase 3
A new pure interface Phase 6
New pure interfaces Phase 10
Phase 11
New pure interfaces Phase 12
Phase 14
A new pure interface Phase 15
New Pure Interfaces CreatedThese are brand-new classes extracted during the refactoring:
Python Backwards-Compatible AliasesThe following aliases were added to the Python bindings so existing scripts continue to work: simplnx.DataObject → simplnx.AbstractDataObject
simplnx.IArray → simplnx.AbstractArray
simplnx.IDataArray → simplnx.AbstractDataArray
simplnx.INeighborList → simplnx.AbstractNeighborList
simplnx.IDataCreationAction → simplnx.AbstractDataCreationActionDead Code Removed
Serialization CompatibilityAll renamed geometry classes preserve their original |
b19af99 to
b741182
Compare
|
I think this is a really important PR and will be worth it in the long run, because it will be great to be able to tell at a glance which classes are interfaces and which are abstract, and it also just makes the whole codebase look more professional, just IMO. |
|
I strongly agree with the renaming scheme, but a couple of the items listed under "potential concerns" sound like glaring red flags. Predominantly among them is the idea of forcing Qt into base simplnx. Things like nxrunner do not and should not require Qt to function. Forcing all of Qt in as a dependency just for easier plugin support seems a bit ham-fisted. The point on undefined behavior on destructor calls is also alarming, and I am not aware of any unit tests we might have that would check for that. The only classes that might trigger that off the top of my head would be in the DataStructure, so it could be important to check that it has detailed enough unit tests. Although, this comment is based purely on the detailed description of the PR and will give a more complete review after looking over it more. |
|
@mmarineBlueQuartz This PR is changing scope and is only about the renaming part. To address your concerns about the original request, Qt's plugin loading was just being used as an example. In no-way-shape-or-form is Qt coming into simplnx. Been down that path once. Not going there again. :-) |
|
I'm not necessarily opposed to the renaming though I am skeptical that it will actually aid us in readability/discoverability. Some new classes were created that alter the existing object hierarchy. Particularly with the geometries having multiple inheritance. We may want to make larger changes to the hierarchy after, but I think it would require more discussion. Therefore I am against those changes. I think this PR should be just renaming. For python there are some backwards compatibility aliases created but not for all. Additionally how long would we maintain those since this is a breaking change. Additionally some new classes were created that don't seem to do anything specifically under json parsing? |
There was a problem hiding this comment.
I poured over everything in the src/simplnx folder (aka the main infrastructure) and the renaming scheme looks good for the most part. A few things I noticed though:
StringStoreseems a little off compared to the other store counterparts. I think it needs to be split into anAbstractandIclass rather than renaming it toIsince it breaks the paradigm. I would also be okay leaving it as just anAbstractclass too- Many of the
Iimplementation files (.cpp) need to be removed and destructor defaults moved to header to bring it inline with typical paradigm used around the project - The comments in the
Geometryfolder need their comments updated to reflect theStatusCode->Result<>changes
Edit: After seeing Jareds feedback I also want to offer that I absolutely agree with the statement about the geometry heirarchy. I think it is definitely worth discussing re-architecting the polymorphism. Multiple inheritance incurs a definitive cost with the runtime vtable look up, so we need to make sure that frequently called methods do not incur this cost. Ideally we remove the diamond problem entirely to ensure there is no runtime cost, but I understand this may not be possible and as such a wider discussion is needed.
There was a problem hiding this comment.
since implementation has all been moved to AbstractGeometry this file should be removed as it is functionally empty
There was a problem hiding this comment.
remove file, implementation moved to abstract class
|
|
||
| #include "simplnx/Common/StringLiteral.hpp" | ||
| #include <optional> |
There was a problem hiding this comment.
| #include <optional> |
unused include
There was a problem hiding this comment.
remove file, no implementation in I files. I files are pure virtual so no .cpp file needed
There was a problem hiding this comment.
remove file, no implementation in I files. I files are pure virtual so no .cpp file needed
There was a problem hiding this comment.
I think that the Abstract and I concept may be confused on this one
There was a problem hiding this comment.
destructor definition is default so move to header. remove file, no implementation in I files. I files are pure virtual so no .cpp file needed
There was a problem hiding this comment.
remove file, no implementation in I files. I files are pure virtual so no .cpp file needed
There was a problem hiding this comment.
move default destructor to header. remove file, no implementation in I files. I files are pure virtual so no .cpp file needed
There was a problem hiding this comment.
move default destructor to header. remove file, no implementation in I files. I files are pure virtual so no .cpp file needed
b741182 to
4498191
Compare
Major Refactoring of the SIMPLNX code base
The point of this PR is to create pure virtual Interfaces, Abstract classes and concreate classes where needed. Extract pure virtual where needed and create the intermediate abstract clases where needed.