Skip to content

Commit

Permalink
refactor: Force custom enum conversion in Json Plugin (#3144)
Browse files Browse the repository at this point in the history
Since we had the same enum conversion problem a second time I propose to disable the default enum serialization via `JSON_DISABLE_ENUM_SERIALIZATION`.

This results in a compile error instead of runtime errors when we actually try to read strings as integers.

I set the flag `PRIVATE` in cmake so it does not sneak into user codebases but the user might face the same problems if the includes are not done correctly.

Not having the enum and the `NLOHMANN_JSON_SERIALIZE_ENUM` in the same place results in these kind of issues. I wonder if we should rather bring them together and use preprocessor flags. That would also guarantee that the right conversion function will be visible all the time.

References:
 - https://json.nlohmann.me/api/macros/json_disable_enum_serialization/
 - https://json.nlohmann.me/api/macros/nlohmann_json_serialize_enum/

Related PRs and issues:
 - #3142
 - #2333
 - #2331
  • Loading branch information
andiwand authored Apr 26, 2024
1 parent 8821e7b commit 234012a
Show file tree
Hide file tree
Showing 5 changed files with 24 additions and 18 deletions.
3 changes: 3 additions & 0 deletions Plugins/Json/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -28,6 +28,9 @@ target_link_libraries(
ActsPluginJson PUBLIC ActsCore)
acts_target_link_libraries_system(
ActsPluginJson PUBLIC nlohmann_json::nlohmann_json)
target_compile_definitions(
ActsPluginJson
PRIVATE JSON_DISABLE_ENUM_SERIALIZATION=1)

install(
TARGETS ActsPluginJson
Expand Down
33 changes: 18 additions & 15 deletions Plugins/Json/include/Acts/Plugins/Json/GridJsonConverter.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,24 @@
// Custom Json encoder/decoders.
namespace Acts {

namespace detail {

/// @cond
NLOHMANN_JSON_SERIALIZE_ENUM(Acts::detail::AxisBoundaryType,
{{Acts::detail::AxisBoundaryType::Bound, "Bound"},
{Acts::detail::AxisBoundaryType::Open, "Open"},
{Acts::detail::AxisBoundaryType::Closed,
"Closed"}})

NLOHMANN_JSON_SERIALIZE_ENUM(Acts::detail::AxisType,
{{Acts::detail::AxisType::Equidistant,
"Equidistant"},
{Acts::detail::AxisType::Variable, "Variable"}})

/// @endcond

} // namespace detail

namespace AxisJsonConverter {

/// Convert an axis to json
Expand Down Expand Up @@ -269,19 +287,4 @@ auto fromJson(const nlohmann::json& jGrid,
}

} // namespace GridJsonConverter

/// @cond
NLOHMANN_JSON_SERIALIZE_ENUM(Acts::detail::AxisBoundaryType,
{{Acts::detail::AxisBoundaryType::Bound, "Bound"},
{Acts::detail::AxisBoundaryType::Open, "Open"},
{Acts::detail::AxisBoundaryType::Closed,
"Closed"}})

NLOHMANN_JSON_SERIALIZE_ENUM(Acts::detail::AxisType,
{{Acts::detail::AxisType::Equidistant,
"Equidistant"},
{Acts::detail::AxisType::Variable, "Variable"}})

/// @endcond

} // namespace Acts
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,6 @@
// can not match our naming guidelines.
namespace Acts {

class VolumeBounds;

void to_json(nlohmann::json& j, const VolumeBounds& bounds);

namespace VolumeBoundsJsonConverter {
Expand Down
3 changes: 2 additions & 1 deletion Plugins/Json/src/ProtoDetectorJsonConverter.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
#include "Acts/Detector/ProtoDetector.hpp"
#include "Acts/Geometry/Extent.hpp"
#include "Acts/Plugins/Json/ExtentJsonConverter.hpp"
#include "Acts/Plugins/Json/SurfaceJsonConverter.hpp"
#include "Acts/Surfaces/Surface.hpp"
#include "Acts/Utilities/BinningData.hpp"

Expand Down Expand Up @@ -42,7 +43,7 @@ void Acts::to_json(nlohmann::json& j, const Acts::ProtoVolume& pv) {
auto& its = pv.internal.value();
nlohmann::json jinternal;
if (its.layerType != Surface::SurfaceType::Other) {
jinternal["layerType"] = static_cast<int>(its.layerType);
jinternal["layerType"] = its.layerType;
}
if (!its.surfaceBinning.empty()) {
writeBinning(jinternal, its.surfaceBinning, "surfaceBinning");
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@
#include "Acts/Detector/ProtoDetector.hpp"
#include "Acts/Geometry/Extent.hpp"
#include "Acts/Plugins/Json/ProtoDetectorJsonConverter.hpp"
#include "Acts/Plugins/Json/SurfaceJsonConverter.hpp"
#include "Acts/Surfaces/Surface.hpp"
#include "Acts/Utilities/BinningData.hpp"
#include "Acts/Utilities/BinningType.hpp"
Expand Down

0 comments on commit 234012a

Please sign in to comment.