Conversation
…nt/pdmVariantEqual Introduces free function customization points to replace PdmValueFieldSpecialization. Default implementations handle Qt-metatype-registered types. Overloads provided for std::vector, std::pair, PdmPointer, AppEnum, FilePath, float, and double.
…riant free functions Replace PdmValueFieldSpecialization class templates in Vec3d, Mat4d, Color3f, Mat3d headers with free function overloads pdmToVariant/pdmFromVariant and full specializations of pdmVariantEqual. Update all call sites in unit tests and application code accordingly.
…free functions Replace PdmValueFieldSpecialization<T>::convert/setFromVariant calls with pdmToVariant/pdmFromVariant free functions in PdmDataValueField, PdmProxyValueField, PdmUiCheckBoxAndTextEditor, and PdmUiValueRangeEditor.
…tEqual free functions Replace all PdmValueFieldSpecialization<T>::convert, setFromVariant, and isEqual calls in PdmUiFieldSpecializationForValueSpec, std::vector/pair/optional specializations, and PdmFieldUiCap::isQVariantDataEqual.
…ining call sites Delete the old policy class header. Update unit tests, comment references, and CMakeLists. All consumers now use pdmToVariant/pdmFromVariant/pdmVariantEqual from cafPdmFieldTraits.h.
Review Summary by QodoModernize field concept: Replace PdmValueFieldSpecialization with free function traits
WalkthroughsDescription• Replace PdmValueFieldSpecialization class templates with ADL-based free functions • Introduce pdmToVariant, pdmFromVariant, pdmVariantEqual customization points • Migrate all call sites in core PDM, UI framework, and application code • Remove deprecated cafInternalPdmValueFieldSpecializations.h header Diagramflowchart LR
Old["PdmValueFieldSpecialization<br/>class templates"]
New["pdmToVariant/<br/>pdmFromVariant/<br/>pdmVariantEqual<br/>free functions"]
CallSites["PDM core, UI framework,<br/>application code"]
Old -->|"migrate to"| New
New -->|"used by"| CallSites
File Changes1. Fwk/AppFwk/cafProjectDataModel/cafPdmCore/cafPdmFieldTraits.h
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
- PdmUiFieldSpecialization<PdmPointer<T>> and <AppEnum<T>> now delegate convert/setFromVariant to pdmToVariant/pdmFromVariant instead of duplicating the same logic - Add list.reserve() in pdmToVariant(std::vector<T>) to avoid repeated reallocations
…n build - Add cafPdmFieldTraitsTest.cpp with 33 tests covering round-trips and pdmVariantEqual for int, double, float, bool, QString, FilePath, AppEnum, vector, pair, and PdmPointer types - Add VariantEqualTest to cafPdmCoreVec3dTest.cpp and cafPdmCoreColor3fTest.cpp - Include cafPdmCvf_UnitTests in RESINSIGHT_INCLUDE_APPFWK_TESTS build
Add std::optional overloads to cafPdmFieldTraits.h so optional fields round-trip correctly and pdmVariantEqual<std::optional<T>> uses the has-value/empty-QVariant convention rather than falling back to a.value<std::optional<T>>() which always returns nullopt. Simplify PdmUiFieldSpecialization<std::optional<T>> to delegate to the free functions. Add 7 tests covering round-trip and equality.
Move type-specific free function overloads into the headers that define those types (cafPdmPointer.h, cafAppEnum.h, cafFilePath.h), so a simple PdmField<int> no longer forces every TU to parse those heavy headers. Replace if constexpr dispatch in pdmVariantEqual with PdmVariantEqualImpl<T> class template partial specializations, which also fixes the extensibility for nested containers (e.g. std::vector<cvf::Vec3d>) since class template partial specialization is allowed where function template partial specialization is not. cafPdmFieldTraits.h now only requires <QVariant> and standard library headers. Each type's equality and conversion logic lives next to its type.
- Remove redundant forward declarations for PdmPointer/AppEnum in
cafInternalPdmFieldTypeSpecializations.h (full headers now included)
- Fix std::list<T> specialization to use pdmToVariant/pdmFromVariant
instead of raw QVariant(*it)/lst[i].value<T>() — consistent with vector
- Remove redundant #include <QVariant> from cafAppEnum.h and cafFilePath.h
(already provided by cafPdmFieldTraits.h)
- Simplify PdmVariantEqualImpl<optional<T>>::equal to use !a.isValid()
instead of allocating toString().remove('"').isEmpty() — pdmToVariant
for nullopt always produces QVariant() (invalid)
…ter and std::optional PdmUiFieldSpecialization<PdmPointer<T>> and PdmUiFieldSpecialization<std::optional<T>> were pure pass-throughs to pdmToVariant/pdmFromVariant/pdmVariantEqual, identical in behavior to the primary template. Removing them lets the primary template handle these types via ADL. Also removes the cafPdmObjectHandle.h include which was only needed for the PdmPointer specialization, reducing include weight for all TUs using cafPdmUiFieldSpecialization.h.
Adds cafPdm_CompileBench, an OBJECT library with five translation units that each define 20 PdmObject subclasses using different field types: basic (int/double/bool/QString), AppEnum, containers (vector/optional/pair), FilePath, and a mixed file covering all types. Unity build is explicitly disabled on the target so each file compiles independently, giving per-file timing via ninja -j1.
…NCLUDE_APPFWK_TESTS
Move <QIcon> and <QPixmap> out of cafIconProvider.h into cafIconProvider.cpp by declaring the destructor out-of-line. This prevents these heavy Qt GUI headers from being pulled into every translation unit that includes cafPdmObject.h. Remove cafPdmOptionItemInfo.h from cafPdmUiItem.h — PdmUiItem does not use PdmOptionItemInfo directly. Move the include to cafPdmUiFieldHandle.h and cafPdmUiObjectHandle.h which actually require it. Fix downstream headers that relied on the transitive include (cafPdmUiItemInfo.h, cafPdmUiFieldEditorHandle.h). Measured improvement over dev branch: 3–7% reduction in bench file compile times, stacking on top of the cafPdmFieldTraits.h include-weight reduction.
…r.h from cafInternalPdmFieldTypeSpecializations.h cafPdmUiItem.h included <string> but std::string is not used anywhere in the header. cafPdmPointer.h was left in cafInternalPdmFieldTypeSpecializations.h after commit bfd12be removed the PdmUiFieldSpecialization<PdmPointer<T>> specialization that required it. Nothing in the remaining code uses PdmPointer, so the include is a leftover. Also remove the unused appEnum parameter name from valueOptions in the AppEnum specialization — it was never referenced in the body.
Remove cafPdmFieldHandle.h: extract the non-template createEnumSubsetKey logic into a new cafAppEnum.cpp as the free function appEnumCreateSubsetKey, allowing PdmFieldHandle to be forward-declared. This removes <functional> and the full PdmFieldHandle class definition (including its capability<T>() template methods) from the header. Remove cafPdmFieldTraits.h: replace with a direct <QVariant> include and a forward declaration of the PdmVariantEqualImpl primary template. The AppEnum-specific pdmToVariant/pdmFromVariant/PdmVariantEqualImpl<AppEnum<T>> overloads remain in the header. This removes 212 lines of type-trait and container-variant template machinery from every TU that includes cafAppEnum.h without the full PDM field stack.
Add <QIcon> to cafPdmUiItemInfo.cpp, cafPdmUiItem.cpp, and cafPdmOptionItemInfo.cpp — these files destroy std::unique_ptr<QIcon> values returned by IconProvider::icon(), which requires the full QIcon definition now that cafIconProvider.h only forward-declares it. Add cafPdmObjectHandle.h to cafPdmUiFieldHandle.cpp — PdmObjectHandle was only forward-declared via cafPdmUiFieldSpecialization.h but the notifyFieldChanged implementation calls methods on it.
These files were relying on transitive includes that were removed or replaced by forward declarations in recent header cleanups.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #834