Enforce parent tag type when creating a pbf_builder submessage

CHANGELOG.md

@@ -14,6 +14,7 @@ This project adheres to [Semantic Versioning](https://semver.org/).
 ### Changed
 
 - The packed varint iterators (`const_varint_iterator`, `const_svarint_iterator`) now only store a single pointer instead of two, which speeds up decoding by 30-150%, depending on varint length. The  constructor now validates once that the byte range ends on a varint boundary and stores only the data pointer. A packed field whose last byte still has its continuation bit set (i.e. a truncated trailing varint, which is illegal) now throws `end_of_buffer_exception` when the iterator range is created (from `get_packed_*()`) rather than later during iteration.
+- The `basic_pbf_builder` constructor for submessages now checks that, when the parent is a `basic_pbf_builder`, the tag has the parent's enum type. Passing a tag of a different enum type or a plain integer tag is now a compile error. If you really need an untyped tag, cast the parent to `basic_pbf_writer&` first. (#155)
 
 ### Fixed
 

doc/tutorial.md

@@ -622,5 +622,15 @@ instantiated using the same `enum class` described above and used exactly
 like the `pbf_writer` class but using the values of the enum instead of bare
 integers.
 
+When creating a `pbf_builder` for a submessage from a parent `pbf_builder`,
+the tag must be a value of the parent's enum type. Using a tag from a different
+enum or a plain integer will not compile:
+
+```cpp
+protozero::pbf_builder<Outer> outer{buffer};
+protozero::pbf_builder<Inner> inner{outer, Outer::sub}; // ok
+protozero::pbf_builder<Inner> wrong{outer, Inner::x};   // compile error
+```
+
 See the `test/t/complex` test case for a complete example using this interface.
 

include/protozero/basic_pbf_builder.hpp

@@ -60,17 +60,32 @@ class basic_pbf_builder : public basic_pbf_writer<TBuffer> {
     }
 
     /**
-     * Construct a pbf_builder for a submessage from the pbf_message or
-     * pbf_writer of the parent message.
+     * Construct a pbf_builder for a submessage from the pbf_writer of the
+     * parent message.
      *
-     * @param parent_writer The parent pbf_message or pbf_writer
+     * @param parent_writer The parent pbf_writer
      * @param tag Tag of the field that will be written
      */
     template <typename P>
     basic_pbf_builder(basic_pbf_writer<TBuffer>& parent_writer, P tag) :
         basic_pbf_writer<TBuffer>{parent_writer, pbf_tag_type(tag)} {
     }
 
+    /**
+     * Construct a pbf_builder for a submessage from the pbf_builder of the
+     * parent message. The tag must be of the enum type of the parent
+     * pbf_builder, otherwise this will not compile.
+     *
+     * @param parent_builder The parent pbf_builder
+     * @param tag Tag of the field that will be written
+     */
+    template <typename P, typename Q>
+    basic_pbf_builder(basic_pbf_builder<TBuffer, P>& parent_builder, Q tag) :
+        basic_pbf_writer<TBuffer>{parent_builder, pbf_tag_type(tag)} {
+        static_assert(std::is_same<P, Q>::value,
+                      "tag must be of the parent builder's enum type");
+    }
+
 /// @cond INTERNAL
 #define PROTOZERO_WRITER_WRAP_ADD_SCALAR(name, type) \
     void add_##name(T tag, type value) { \

test/CMakeLists.txt

@@ -10,6 +10,7 @@ include_directories(SYSTEM "${CMAKE_CURRENT_SOURCE_DIR}/catch")
 include_directories("${CMAKE_CURRENT_SOURCE_DIR}/include")
 
 add_subdirectory(unit)
+add_subdirectory(compile_fail)
 
 set(TEST_DIRS alignment
               bool

test/compile_fail/CMakeLists.txt

@@ -0,0 +1,37 @@
+#-----------------------------------------------------------------------------
+#
+#  CMake config
+#
+#  protozero compile failure tests
+#
+#  Each source file in here must fail to compile with the expected error
+#  message. This is used to test that misuse of the API is caught at compile
+#  time, for instance by a static_assert.
+#
+#  To add a test, create a new source file and add its name (without the
+#  .cpp suffix) to COMPILE_FAIL_TESTS below.
+#
+#-----------------------------------------------------------------------------
+
+set(COMPILE_FAIL_TESTS builder_parent_wrong_enum
+                       builder_parent_integer_tag)
+
+foreach(_test IN LISTS COMPILE_FAIL_TESTS)
+    # Compile-only target (no main() needed), excluded from the normal build.
+    add_library(compile_fail_${_test} OBJECT EXCLUDE_FROM_ALL ${_test}.cpp)
+    set_target_properties(compile_fail_${_test} PROPERTIES EXCLUDE_FROM_DEFAULT_BUILD ON)
+
+    # The test builds the target above, so its output is the compiler output.
+    add_test(NAME compile_fail_${_test}
+             COMMAND ${CMAKE_COMMAND} --build ${CMAKE_BINARY_DIR}
+                                      --target compile_fail_${_test}
+                                      --config $<CONFIG>)
+
+    # Pass only if the expected error shows up (the exit code is ignored).
+    # Unlike WILL_FAIL, this doesn't accept unrelated compile errors.
+    set_tests_properties(compile_fail_${_test} PROPERTIES
+                         PASS_REGULAR_EXPRESSION "tag must be of the parent builder's enum type")
+endforeach()
+
+
+#-----------------------------------------------------------------------------

test/compile_fail/builder_parent_integer_tag.cpp

@@ -0,0 +1,22 @@
+
+// This file must not compile: A pbf_builder parent requires a tag of its enum
+// type, not a plain integer.
+
+#include <protozero/pbf_builder.hpp>
+
+#include <string>
+
+enum class Outer : protozero::pbf_tag_type {
+    sub = 1
+};
+
+enum class Inner : protozero::pbf_tag_type {
+    value = 2
+};
+
+void test() {
+    std::string buffer;
+    protozero::pbf_builder<Outer> outer{buffer};
+    protozero::pbf_builder<Inner> inner{outer, 1};
+}
+

test/compile_fail/builder_parent_wrong_enum.cpp

@@ -0,0 +1,22 @@
+
+// This file must not compile: The tag used to open a submessage must have the
+// enum type of the parent pbf_builder.
+
+#include <protozero/pbf_builder.hpp>
+
+#include <string>
+
+enum class Outer : protozero::pbf_tag_type {
+    sub = 1
+};
+
+enum class Inner : protozero::pbf_tag_type {
+    value = 2
+};
+
+void test() {
+    std::string buffer;
+    protozero::pbf_builder<Outer> outer{buffer};
+    protozero::pbf_builder<Inner> inner{outer, Inner::value};
+}
+

test/unit/CMakeLists.txt

@@ -9,6 +9,7 @@
 set(UNIT_TESTS data_view
                basic
                buffer
+               builder
                endian
                exceptions
                iterators

test/unit/test_builder.cpp

@@ -0,0 +1,49 @@
+
+#include <test.hpp>
+
+#include <string>
+
+namespace {
+
+enum class Outer : protozero::pbf_tag_type {
+    sub = 1
+};
+
+enum class Inner : protozero::pbf_tag_type {
+    value = 2
+};
+
+const std::string expected{"\x0a\x02\x10\x2a"};
+
+} // anonymous namespace
+
+TEST_CASE("pbf_builder submessage from pbf_builder parent with matching tag type") {
+    std::string buffer;
+    {
+        protozero::pbf_builder<Outer> outer{buffer};
+        protozero::pbf_builder<Inner> inner{outer, Outer::sub};
+        inner.add_uint32(Inner::value, 42);
+    }
+    REQUIRE(buffer == expected);
+}
+
+TEST_CASE("pbf_builder submessage from pbf_writer parent with integer tag") {
+    std::string buffer;
+    {
+        protozero::pbf_writer outer{buffer};
+        protozero::pbf_builder<Inner> inner{outer, 1};
+        inner.add_uint32(Inner::value, 42);
+    }
+    REQUIRE(buffer == expected);
+}
+
+TEST_CASE("pbf_builder submessage from pbf_builder parent cast to pbf_writer") {
+    std::string buffer;
+    {
+        protozero::pbf_builder<Outer> outer{buffer};
+        protozero::pbf_builder<Inner> inner{static_cast<protozero::pbf_writer&>(outer), 1};
+        inner.add_uint32(Inner::value, 42);
+    }
+    REQUIRE(buffer == expected);
+}
+