From f5ec6905a1f74bf39abeb1d477796fed4e5431ff Mon Sep 17 00:00:00 2001 From: Thomas Nixon Date: Tue, 16 Jan 2024 17:09:34 +0000 Subject: [PATCH 1/9] mark enum bitmask operators as constexpr --- include/adm/detail/enum_bitmask.hpp | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) diff --git a/include/adm/detail/enum_bitmask.hpp b/include/adm/detail/enum_bitmask.hpp index d2a9bdd5..d36fe237 100644 --- a/include/adm/detail/enum_bitmask.hpp +++ b/include/adm/detail/enum_bitmask.hpp @@ -18,7 +18,7 @@ namespace adm { template typename std::enable_if::value, - Enum>::type + Enum>::type constexpr operator|(Enum lhs, Enum rhs) { using T = typename std::underlying_type::type; return static_cast(static_cast(lhs) | static_cast(rhs)); @@ -26,7 +26,7 @@ operator|(Enum lhs, Enum rhs) { template typename std::enable_if::value, - Enum>::type + Enum>::type constexpr operator&(Enum lhs, Enum rhs) { using T = typename std::underlying_type::type; return static_cast(static_cast(lhs) & static_cast(rhs)); @@ -34,7 +34,7 @@ operator&(Enum lhs, Enum rhs) { template typename std::enable_if::value, - Enum>::type + Enum>::type constexpr operator^(Enum lhs, Enum rhs) { using T = typename std::underlying_type::type; return static_cast(static_cast(lhs) ^ static_cast(rhs)); @@ -42,7 +42,7 @@ operator^(Enum lhs, Enum rhs) { template typename std::enable_if::value, - Enum>::type + Enum>::type constexpr operator~(Enum lhs) { using T = typename std::underlying_type::type; return static_cast(~static_cast(lhs)); @@ -50,7 +50,7 @@ operator~(Enum lhs) { template typename std::enable_if::value, - Enum&>::type + Enum&>::type constexpr operator|=(Enum& lhs, Enum rhs) { using T = typename std::underlying_type::type; lhs = static_cast(static_cast(lhs) | static_cast(rhs)); @@ -59,7 +59,7 @@ operator|=(Enum& lhs, Enum rhs) { template typename std::enable_if::value, - Enum&>::type + Enum&>::type constexpr operator&=(Enum& lhs, Enum rhs) { using T = typename std::underlying_type::type; lhs = static_cast(static_cast(lhs) & static_cast(rhs)); @@ -68,7 +68,7 @@ operator&=(Enum& lhs, Enum rhs) { template typename std::enable_if::value, - Enum&>::type + Enum&>::type constexpr operator^=(Enum& lhs, Enum rhs) { using T = typename std::underlying_type::type; lhs = static_cast(static_cast(lhs) ^ static_cast(rhs)); From fabf37589bd8e6c8985cfe1cc37ad3e0af4179de Mon Sep 17 00:00:00 2001 From: Thomas Nixon Date: Tue, 16 Jan 2024 17:57:56 +0000 Subject: [PATCH 2/9] correct ordering of extern templates and base classes --- include/adm/elements/position_offset.hpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/include/adm/elements/position_offset.hpp b/include/adm/elements/position_offset.hpp index 48a4a141..5aa672c6 100644 --- a/include/adm/elements/position_offset.hpp +++ b/include/adm/elements/position_offset.hpp @@ -14,15 +14,6 @@ namespace adm { namespace detail { - using SphericalPositionOffsetBase = - HasParameters, - OptionalParameter, - OptionalParameter>; - - using CartesianPositionOffsetBase = - HasParameters, OptionalParameter, - OptionalParameter>; - extern template class ADM_EXPORT_TEMPLATE_METHODS OptionalParameter; extern template class ADM_EXPORT_TEMPLATE_METHODS @@ -37,6 +28,15 @@ namespace adm { extern template class ADM_EXPORT_TEMPLATE_METHODS OptionalParameter; + using SphericalPositionOffsetBase = + HasParameters, + OptionalParameter, + OptionalParameter>; + + using CartesianPositionOffsetBase = + HasParameters, OptionalParameter, + OptionalParameter>; + } // namespace detail /// @brief Tag for SphericalPositionOffset class From 36ca41498dcb6f52c274c2e1f1d0dd3040a23e9c Mon Sep 17 00:00:00 2001 From: Thomas Nixon Date: Fri, 19 Jan 2024 12:33:21 +0000 Subject: [PATCH 3/9] remove automatic tag creation from ADD_TRAIT this shortens the name of some tags, from adm::detail::ParameterTraits<...>::SomeTag to just SomeTag in most of the places where this is used, an unused tag type was created anyway (sometimes with the wrong name), so using that is probably clearer --- include/adm/detail/type_traits.hpp | 1 - include/adm/elements/audio_block_format_direct_speakers.hpp | 6 ++++-- include/adm/elements/dialogue.hpp | 2 +- include/adm/elements/loudness_metadata.hpp | 1 + include/adm/elements/position.hpp | 1 + include/adm/elements/position_offset.hpp | 1 + 6 files changed, 8 insertions(+), 4 deletions(-) diff --git a/include/adm/detail/type_traits.hpp b/include/adm/detail/type_traits.hpp index 372bd29f..9f4a98ac 100644 --- a/include/adm/detail/type_traits.hpp +++ b/include/adm/detail/type_traits.hpp @@ -9,7 +9,6 @@ namespace adm { namespace detail { \ template <> \ struct ParameterTraits { \ - struct TAG {}; \ typedef TAG tag; \ }; \ } diff --git a/include/adm/elements/audio_block_format_direct_speakers.hpp b/include/adm/elements/audio_block_format_direct_speakers.hpp index a857e062..bb487dd6 100644 --- a/include/adm/elements/audio_block_format_direct_speakers.hpp +++ b/include/adm/elements/audio_block_format_direct_speakers.hpp @@ -22,15 +22,17 @@ namespace adm { struct SpeakerLabelTag {}; /// @brief NamedType for a speaker label using SpeakerLabel = detail::NamedType; - /// @brief NamedType for all speaker labels of an AudioBlockFormat + /// @brief type for all speaker labels of an AudioBlockFormat using SpeakerLabels = std::vector; + /// @brief Tag for SpeakerLabels + struct SpeakerLabelsTag {}; ADD_TRAIT(SpeakerLabels, SpeakerLabelsTag); /// @brief NamedType for speaker position in an AudioBlockFormat using SpeakerPosition = boost::variant; struct SpeakerPositionTag {}; - ADD_TRAIT(SpeakerPosition, SpeakerPostionTag); + ADD_TRAIT(SpeakerPosition, SpeakerPositionTag); namespace detail { using AudioBlockFormatDirectSpeakersBase = diff --git a/include/adm/elements/dialogue.hpp b/include/adm/elements/dialogue.hpp index 337dc470..9016b255 100644 --- a/include/adm/elements/dialogue.hpp +++ b/include/adm/elements/dialogue.hpp @@ -101,7 +101,7 @@ namespace adm { typedef boost::variant ContentKind; - + struct ContentKindTag {}; ADD_TRAIT(ContentKind, ContentKindTag); } // namespace adm diff --git a/include/adm/elements/loudness_metadata.hpp b/include/adm/elements/loudness_metadata.hpp index e0ef37e1..1f1ba9bf 100644 --- a/include/adm/elements/loudness_metadata.hpp +++ b/include/adm/elements/loudness_metadata.hpp @@ -54,6 +54,7 @@ namespace adm { struct LoudnessMetadataTag {}; using LoudnessMetadatas = std::vector; + struct LoudnessMetadatasTag {}; ADD_TRAIT(LoudnessMetadatas, LoudnessMetadatasTag); class LoudnessMetadata { diff --git a/include/adm/elements/position.hpp b/include/adm/elements/position.hpp index 2b86d598..29943376 100644 --- a/include/adm/elements/position.hpp +++ b/include/adm/elements/position.hpp @@ -221,6 +221,7 @@ namespace adm { ///@brief Type to hold a SphericalPosition or CartesianPosition typedef boost::variant Position; + struct PositionTag {}; ADD_TRAIT(Position, PositionTag); // ---- Free functions ---- // diff --git a/include/adm/elements/position_offset.hpp b/include/adm/elements/position_offset.hpp index 5aa672c6..d6923132 100644 --- a/include/adm/elements/position_offset.hpp +++ b/include/adm/elements/position_offset.hpp @@ -140,6 +140,7 @@ namespace adm { /// @brief Type to hold a SphericalPositionOffset or CartesianPositionOffset typedef boost::variant PositionOffset; + struct PositionOffsetTag {}; ADD_TRAIT(PositionOffset, PositionOffsetTag); // ---- Free functions ---- // From abdb90d734da15467caba0055ed4f383c7654476 Mon Sep 17 00:00:00 2001 From: Thomas Nixon Date: Thu, 18 Jan 2024 11:53:54 +0000 Subject: [PATCH 4/9] auto_base: fix VariantParameter docs --- include/adm/detail/auto_base.hpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/include/adm/detail/auto_base.hpp b/include/adm/detail/auto_base.hpp index d8d07262..a4bb5982 100644 --- a/include/adm/detail/auto_base.hpp +++ b/include/adm/detail/auto_base.hpp @@ -326,7 +326,7 @@ namespace adm { /// When using this with OptionalParameter, the following classes should /// be explicitly instantiated: /// - OptionalParameter (not VariantParameter<...>) - /// - One VariantParameter, T> for each T in V. + /// - One VariantTypeParameter, T> for each T in V. template using VariantParameter = typename VariantParameterHelper< VariantParam, typename VariantParam::ParameterType>::type; From d0a35c8475d24d4466c8af14a9f2c824d67f0048 Mon Sep 17 00:00:00 2001 From: Thomas Nixon Date: Thu, 18 Jan 2024 12:12:41 +0000 Subject: [PATCH 5/9] auto_base: add tests for variants --- tests/auto_base_tests.cpp | 47 ++++++++++++++++++++++++++++++++++++++- 1 file changed, 46 insertions(+), 1 deletion(-) diff --git a/tests/auto_base_tests.cpp b/tests/auto_base_tests.cpp index cd99d8e0..500b5a7f 100644 --- a/tests/auto_base_tests.cpp +++ b/tests/auto_base_tests.cpp @@ -26,6 +26,21 @@ namespace adm { using Vectors = std::vector; ADD_TRAIT(Vectors, VectorsTag); + struct VariantATag {}; + struct VariantA { + using tag = VariantATag; + int x; + }; + struct VariantBTag {}; + struct VariantB { + using tag = VariantBTag; + int x; + }; + + struct VariantTag {}; + using Variant = boost::variant; + ADD_TRAIT(Variant, VariantTag); + namespace detail { template <> Default getDefault() { @@ -36,10 +51,14 @@ namespace adm { template class OptionalParameter; template class DefaultParameter; template class VectorParameter; + template class OptionalParameter; + template class VariantTypeParameter, VariantA>; + template class VariantTypeParameter, VariantB>; using Base = HasParameters, OptionalParameter, - DefaultParameter, VectorParameter>; + DefaultParameter, VectorParameter, + VariantParameter>>; }; // namespace detail // standard wrapper around Base providing templated methods. not really @@ -166,3 +185,29 @@ TEST_CASE("vector") { REQUIRE(e.get() == Vectors{}); REQUIRE(!e.isDefault()); } + +TEST_CASE("variant") { + TestElement e; + REQUIRE(!e.has()); + REQUIRE(!e.isDefault()); + + // set from the variant type + e.set(Variant{VariantA{1}}); + REQUIRE(boost::get(e.get()).x == 1); + REQUIRE(e.has()); + REQUIRE(!e.has()); + REQUIRE(e.get().x == 1); + + e.set(Variant{VariantB{2}}); + REQUIRE(boost::get(e.get()).x == 2); + REQUIRE(!e.has()); + REQUIRE(e.has()); + REQUIRE(e.get().x == 2); + + // set from the contained types + e.set(VariantA{3}); + REQUIRE(e.get().x == 3); + + e.set(VariantB{4}); + REQUIRE(e.get().x == 4); +} From fad18d8c005a979c17123adc132127de0e203d5f Mon Sep 17 00:00:00 2001 From: Thomas Nixon Date: Tue, 16 Jan 2024 17:14:41 +0000 Subject: [PATCH 6/9] switch to C++17 the minimum cmake version is now 3.8, which is still ancient --- CMakeLists.txt | 2 +- src/CMakeLists.txt | 11 ++--------- 2 files changed, 3 insertions(+), 10 deletions(-) diff --git a/CMakeLists.txt b/CMakeLists.txt index 3ec58f52..96f86596 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -1,4 +1,4 @@ -cmake_minimum_required(VERSION 3.5) +cmake_minimum_required(VERSION 3.8) ############################################################ # set global paths diff --git a/src/CMakeLists.txt b/src/CMakeLists.txt index 931a0dfb..515bd16a 100644 --- a/src/CMakeLists.txt +++ b/src/CMakeLists.txt @@ -98,16 +98,9 @@ if(ADM_HIDE_INTERNAL_SYMBOLS AND BUILD_SHARED_LIBS) endif() ############################################################ -# enable C++14 support +# enable C++17 support ############################################################ -if (${CMAKE_VERSION} VERSION_LESS "3.8.0") - # Note: this is not a complete list of c++ features required by libadm. - # What we want is C++14 support and this is a simple way to trigger - # this for CMake < 3.8 - target_compile_features(adm PUBLIC cxx_generic_lambdas) -else() - target_compile_features(adm PUBLIC cxx_std_14) -endif() +target_compile_features(adm PUBLIC cxx_std_17) set_target_properties(adm PROPERTIES CXX_EXTENSIONS OFF) include(GenerateExportHeader) From e77220b2ac54cc0212fcab2c1854c1aaeaaf0383 Mon Sep 17 00:00:00 2001 From: Thomas Nixon Date: Tue, 16 Jan 2024 17:36:10 +0000 Subject: [PATCH 7/9] auto_base: use C++17 parameter pack expansion in using --- include/adm/detail/auto_base.hpp | 146 +++--------------------- include/adm/detail/auto_base_detail.hpp | 116 +++++++++++++++---- 2 files changed, 109 insertions(+), 153 deletions(-) diff --git a/include/adm/detail/auto_base.hpp b/include/adm/detail/auto_base.hpp index a4bb5982..1db77418 100644 --- a/include/adm/detail/auto_base.hpp +++ b/include/adm/detail/auto_base.hpp @@ -27,130 +27,12 @@ namespace adm { // This is done using multiple inheritance: given a set of *Base classes, // we can make a derived class using HasParameters which inherits from all // of them. - // - // The problem this presents is that the derived class needs a using - // declaration pointing to each method in the base classes, in order to - // bring them into one overload set, but we can't have a using declaration - // which points to a method which doesn't exist. - // - // To make this work, each *Base class defines some flags (defined in - // Flags) which says which sets of methods it implements. When Combine is - // used to combine two *Base classes together, these flags are or-ed in the - // resulting class, and the correct specialisations of the Combine* classes - // are selected by and-ing the flags (we only need the using declarations - // if both bases have the methods defined). - // - // To enable all combinations without making the dreaded diamond, these are - // chained, with the actual inheritance happening in CombineRaw. When - // combining A and B we have an inheritance hierarchy like: - // - // Combine - // -> ApplyIf, X> - // -> ApplyIf, Y> - // -> CombineRaw - // -> A - // -> B - // - // where X and Y are the flags controlling whether Combine* is applied or - // not. - // - // Combine is used by HasParameters, which recursively combines many *Base - // classes. - // - // For variant types, there is one base class which implements the methods - // for the variant type (e.g. OptionalParameter>), and one - // base class for each of the types T in the variant, e.g. - // VariantTypeParameter. - // - // VariantTypeParameter classes must inherit from OptionalParameter in - // order to access the variant (through the get/set methods). This could be - // implemented by templating VariantTypeParameter on the list of types of - // the variant, and each inheriting from the next, but each of these - // classes is to be explicitly instantiated, and the meaning of these - // parameters in the explicit instantiations would not be clear. - // - // Instead, each VariantTypeParameter inherits from OptionalParameter, and - // virtual inheritance is used to avoid ambiguity. These are then combined - // together in VariantParameter using HasParameters, so that only one type - // has to be added to the base class list to cover the whole variant. - // - // For clarity, if we have P = OptionalParameter>, then the - // inheritance structure looks like: - // - // VariantParameter

- // -> VariantTypeParameter - // -> P - // -> VariantTypeParameter - // -> P #ifndef IN_DOXYGEN - - struct Flags { - static constexpr bool has_get_set_has = false; - static constexpr bool has_isDefault_unset = false; - static constexpr bool has_add_remove = false; - }; - - /// a subclass of Base, with using declarations for set, get and has in A - /// and B - template - struct CombineGetSetHas : public Base { - using A::get; - using B::get; - - using A::set; - using B::set; - - using A::has; - using B::has; - }; - - /// a subclass of Base, with using declarations for isDefault and unset in A - /// and B - template - struct CombineIsDefaultUnset : public Base { - using A::isDefault; - using B::isDefault; - - using A::unset; - using B::unset; - }; - - /// a subclass of Base, with using declarations for add and remove in A and - /// B - template - struct CombineAddRemove : public Base { - using A::add; - using B::add; - - using A::remove; - using B::remove; - }; - - /// a subclass of A and B, with methods according to their Flags - template - struct Combine - : public ApplyIf< - A::has_add_remove && B::has_add_remove, CombineAddRemove, A, B, - ApplyIf>>> { - static constexpr bool has_get_set_has = - A::has_get_set_has || B::has_get_set_has; - static constexpr bool has_isDefault_unset = - A::has_isDefault_unset || B::has_isDefault_unset; - static constexpr bool has_add_remove = - A::has_add_remove || B::has_add_remove; - }; - /// make a class derived from the given base classes, combining the /// get, set, has, isDefault and unset overloads - template - struct HasParameters : public Combine> {}; - - template - struct HasParameters : public B {}; + template + using HasParameters = Combine>; /// Get the default value of T parameters. Specialise this to add custom /// defaults. @@ -163,10 +45,10 @@ namespace adm { /// combine these together using HasParameters template ::tag> - class RequiredParameter : public Flags { + class RequiredParameter { public: using ParameterType = T; - static constexpr bool has_get_set_has = true; + static constexpr Flags flags = Flags::HAS_GET_SET_HAS; ADM_BASE_EXPORT T get(Tag) const { return value_; } ADM_BASE_EXPORT void set(T value) { value_ = std::move(value); } @@ -181,11 +63,11 @@ namespace adm { /// HasParameters template ::tag> - class OptionalParameter : public Flags { + class OptionalParameter { public: using ParameterType = T; - static constexpr bool has_get_set_has = true; - static constexpr bool has_isDefault_unset = true; + static constexpr Flags flags = + Flags::HAS_GET_SET_HAS | Flags::HAS_ISDEFAULT_UNSET; ADM_BASE_EXPORT T get(Tag) const { return value_.get(); } ADM_BASE_EXPORT void set(T value) { value_ = std::move(value); } @@ -202,11 +84,11 @@ namespace adm { /// combine these together using HasParameters template ::tag> - class DefaultParameter : public Flags { + class DefaultParameter { public: using ParameterType = T; - static constexpr bool has_get_set_has = true; - static constexpr bool has_isDefault_unset = true; + static constexpr Flags flags = + Flags::HAS_GET_SET_HAS | Flags::HAS_ISDEFAULT_UNSET; ADM_BASE_EXPORT T get(Tag) const { return boost::get_optional_value_or(value_, getDefault()); @@ -232,14 +114,14 @@ namespace adm { /// associated with. template ::tag> - class VectorParameter : public Flags { + class VectorParameter { using Value = typename T::value_type; public: using ParameterType = T; - static constexpr bool has_get_set_has = true; - static constexpr bool has_isDefault_unset = true; - static constexpr bool has_add_remove = true; + static constexpr Flags flags = Flags::HAS_GET_SET_HAS | + Flags::HAS_ISDEFAULT_UNSET | + Flags::HAS_ADD_REMOVE; ADM_BASE_EXPORT T get(Tag) const { return value_; } ADM_BASE_EXPORT void set(T value) { value_ = std::move(value); } diff --git a/include/adm/detail/auto_base_detail.hpp b/include/adm/detail/auto_base_detail.hpp index a28fb2b6..e78bcb53 100644 --- a/include/adm/detail/auto_base_detail.hpp +++ b/include/adm/detail/auto_base_detail.hpp @@ -1,31 +1,105 @@ +#pragma once + +#include "adm/detail/enum_bitmask.hpp" +#include namespace adm { namespace detail { - /// Combine A and B using F if defined_in_both, otherwise Base - template class F, - typename A, typename B, typename Base> - struct ApplyIfT; - - template