Skip to content

Refactor application parameters dynamic_cast pattern

Victor Poughon requested to merge refactor-dynamiccast into develop

Summary

Refactor otbWrapperApplication.cxx and application parameter's:

  • Add GetTypes/ToInt/FromInt/ToString/FromString/etc virtual methods in Parameter to replace dynamic_casts
  • Refactor NumericalParameter & its test
    • Use boost::optional instead of boost::any
    • Use inheritance to avoid soft typedef type issue
    • Simplify RAMParameter & RadiusParameter
  • Factorize error checking in the single dynamic_cast pattern
  • Fix const-correctness of some methods
  • Move some inline code to the .cxx file

Fix 5 bugs:

  • Fix incorrect StringList usage in DSFuzzyModelEstimation
  • Fix incorrect usage of SetDefaultParameterFloat in SARPolarSynth
  • Fix incorrect type in SampleAugmentation
  • Type error in ExtractROI
  • Incorrect usage of SetParameterFloat in Bug804.py

Rationale

Heavy usage of dynamic_cast is an anti-pattern. Using virtual functions is more idiomatic and much less error prone. The code is simpler to read and easier to maintain. Using this refactoring I found 5 bugs that were silent before (because there was no error checking).

Related to #1531 (closed), #1842 (closed)

Implementation Details

The key idea is that the base class Parameter defines virtual functions for type conversion with a default implementation that raises an exception. Derived classes can override this implementation if they are able to provide the conversion.

An important point is that parameter type conversion and error checking is now much more strict. For example SetParameterFloat on a parameter of type ParameterType_Int will now result in a runtime error:

itk::ERROR: Cannot convert parameter 'sizex' of type Int to/from type Float

TODO in future MR:

  • Continue refactoring with other types (see otbWrapperParameter.h)
  • Improve error message (remove "itk::ERROR")

Copyright

The copyright owner is CNES and has signed the ORFEO ToolBox Contributor License Agreement.

Check before merging:

  • All discussions are resolved
  • At least 2 👍 votes from core developers, no 👎 vote.
  • The feature branch is (reasonably) up-to-date with the base branch
  • Dashboard is green
  • Copyright owner has signed the ORFEO ToolBox Contributor License Agreement
Edited by Victor Poughon

Merge request reports