(Const)NodeRef: add .at(), improve coverage, verify error handling

README.md

@@ -844,6 +844,12 @@ ryml:
     because this may cost up to 10% in processing time.
   * `RYML_DEFAULT_CALLBACKS=ON/OFF`. Enable/disable ryml's default
     implementation of error and allocation callbacks. Defaults to `ON`.
+  * `RYML_DEFAULT_CALLBACK_USES_EXCEPTIONS=ON/OFF` - Enable/disable
+    the same-named macro, which will make the default error handler
+    provided by ryml throw a `std::runtime_error` exception.
+  * `RYML_USE_ASSERT` - enable assertions in the code regardless of
+    build type. This is disabled by default. Failed assertions will
+    trigger a call to the error callback.
   * `RYML_STANDALONE=ON/OFF`. ryml uses
     [c4core](https://github.com/biojppm/c4core), a C++ library with low-level
     multi-platform utilities for C++. When `RYML_STANDALONE=ON`, c4core is

changelog/current.md

@@ -1,13 +1,13 @@
+### Error handling
 
-### Fixes
+Fix major error handling problem reported in [#389](https://github.com/biojppm/rapidyaml/issues/389) ([PR#411](https://github.com/biojppm/rapidyaml/pull/411)):
 
-- Fix major error handling problem reported in [#389](https://github.com/biojppm/rapidyaml/issues/389) ([PR#411](https://github.com/biojppm/rapidyaml/pull/411)):
   - The `NodeRef` and `ConstNodeRef` classes had many methods marked `noexcept` that were doing assertions which could throw exceptions, causing an abort instead of a throw whenever the assertion called an exception-throwing error callback.
   - Also, this problem was compounded by exceptions being enabled in every build type -- despite the intention to have them only in debug builds. There was a problem in the preprocessor code to enable assertions which led to assertions being enabled in release builds even when `RYML_USE_ASSERT` was defined to 0. Thanks to @jdrouhard for reporting this.
-  - Although the code is and was extensively tested, the testing was addressing mostly the happy path. In the fix, I added tests to ensure that the error behavior is as intended.
-  - Together with this changeset, a major revision was carried out of the asserting/checking status of each function in the node classes. In most cases, assertions were added to cases that were missing them. So **beware** - user code that was invalid will now assert or error out. Also, assertions and checks are now directed as much as possible to the callbacks of the closest scope, ie if a tree has custom callbacks, errors should go through those callbacks.
+  - Although the code is and was extensively tested, the testing was addressing mostly the happy path. Tests were added to ensure that the error behavior is as intended.
+  - Together with this changeset, a major revision was carried out of the asserting/checking status of each function in the node classes. In most cases, assertions were added to functions cases that were missing them. So **beware** - user code that was invalid will now assert or error out. Also, assertions and checks are now directed as much as possible to the callbacks of the closest scope, ie if a tree has custom callbacks, errors should go through those callbacks.
   - Also, the intended assertion behavior is now in place: *no assertions in release builds*. **Beware** as well - user code which was relying on this will now silently succeed and return garbage in release builds. See the next points, which may help:
-  - Added new methods to the node class:
+  - Added new methods to the `NodeRef`/`ConstNodeRef` classes:
     ```c++
     /** Distinguish between a valid seed vs a valid non-seed ref. */
     bool readable() const { return valid() && !is_seed(); }
@@ -21,25 +21,31 @@
      * readable, or when it is not a map. This behaviour is similar to
      * std::vector::at(), but the error consists in calling the error
      * callback instead of directly raising an exception. */
-    ConstNodeRef ConstNodeRef::at(csubstr key) const;
-    ConstNodeRef NodeRef::at(csubstr key) const;
+    ConstNodeRef at(csubstr key) const;
+    /** Likewise, but return a seed node when the key is not found */
+    ConstNodeRef at(csubstr key);
 
     /** Get a child by position, with error checking; complexity is
      * O(pos).
      *
      * Behaves as operator[](size_t) const, but always raises an error
      * (even when RYML_USE_ASSERT is set to false) when the returned
      * node does not exist, or when this node is not readable, or when
-     * it is not a map. This behaviour is similar to
+     * it is not a container. This behaviour is similar to
      * std::vector::at(), but the error consists in calling the error
      * callback instead of directly raising an exception. */
-    ConstNodeRef ConstNodeRef::at(size_t pos) const;
-   ConstNodeRef NodeRef::at(size_t pos) const;
-   ```
+    ConstNodeRef at(size_t pos) const;
+    /** Likewise, but return a seed node when pos is not found */
+    ConstNodeRef at(csubstr key);
+    ```
   - Added macros and respective cmake options to control error handling:
     - `RYML_USE_ASSERT` - enable assertions regardless of build type. This is disabled by default.
     - `RYML_DEFAULT_CALLBACK_USES_EXCEPTIONS` - defines the same macro, which will make the default error handler provided by ryml throw exceptions instead of calling `std::abort()`. This is disabled by default.
   - Also, `RYML_DEBUG_BREAK()` is now enabled only if `RYML_DBG` is defined, as reported in [#362](https://github.com/biojppm/rapidyaml/issues/362).
+
+
+### More fixes
+
 - Fix [#390](https://github.com/biojppm/rapidyaml/pull/390) - `csubstr::first_real_span()` failed on scientific numbers with one digit in the exponent.
 - Fix [#361](https://github.com/biojppm/rapidyaml/pull/361) - parse error on map scalars containing `:` and starting on the next line:
   ```yaml
@@ -55,3 +61,9 @@
 - Fix [#373](https://github.com/biojppm/rapidyaml/issues/373) - false parse error with empty quoted keys in block-style map ([PR#374](https://github.com/biojppm/rapidyaml/pull/374)).
 - Fix [#356](https://github.com/biojppm/rapidyaml/issues/356) - fix overzealous check in `emit_as()`. An id may be larger than the tree's size, eg when nodes were removed.
 ([PR#357](https://github.com/biojppm/rapidyaml/pull/357)).
+
+
+### Thanks
+
+- @Neko-Box-Coder
+- @jdrouhard

samples/quickstart.cpp

@@ -4042,13 +4042,9 @@ void sample_error_handler()
     // crash.
     ryml::set_callbacks(errh.callbacks());
     errh.check_effect(/*committed*/true);
-    bool had_parse_error = true;
-    errh.check_error_occurs([&had_parse_error]{
-        had_parse_error = true;
+    errh.check_error_occurs([&]{
         ryml::Tree tree = ryml::parse_in_arena("errorhandler.yml", "[a: b\n}");
-        had_parse_error = false; // this line is not executed
     });
-    CHECK(had_parse_error);
     ryml::set_callbacks(errh.defaults); // restore defaults.
     errh.check_effect(/*committed*/false);
 }

src/c4/yml/node.hpp

@@ -323,6 +323,14 @@ struct RoNodeMethods
     C4_ALWAYS_INLINE size_t num_other_siblings() const RYML_NOEXCEPT { _C4RV(); return tree_->num_other_siblings(id_); }
     C4_ALWAYS_INLINE size_t sibling_pos(ConstImpl const& n) const RYML_NOEXCEPT { _C4RV(); _RYML_CB_ASSERT(tree_->callbacks(), n.readable()); return tree_->child_pos(tree_->parent(id_), n.m_id); }
 
+    /** @} */
+
+public:
+
+    /** @name square_brackets
+     * operator[] */
+    /** @{ */
+
     /** Find child by key; complexity is O(num_children).
      *
      * Returns the requested node, or an object in seed state if no
@@ -332,10 +340,11 @@ struct RoNodeMethods
      * to the tree provided that its create() method is called prior
      * to writing, which happens in most modifying methods in
      * NodeRef. It is the caller's responsibility to verify that the
-     * returned node is readable before subsequently using it.
+     * returned node is readable before subsequently using it to read
+     * from the tree.
      *
      * @warning the calling object must be readable. This precondition
-     * is asserted. This assertion is performed only if @ref
+     * is asserted. The assertion is performed only if @ref
      * RYML_USE_ASSERT is set to true. As with the non-const overload,
      * it is UB to call this method if the node is not readable.
      *
@@ -357,10 +366,11 @@ struct RoNodeMethods
      * to the tree provided that its create() method is called prior
      * to writing, which happens in most modifying methods in
      * NodeRef. It is the caller's responsibility to verify that the
-     * returned node is readable before subsequently using it.
+     * returned node is readable before subsequently using it to read
+     * from the tree.
      *
      * @warning the calling object must be readable. This precondition
-     * is asserted. This assertion is performed only if @ref
+     * is asserted. The assertion is performed only if @ref
      * RYML_USE_ASSERT is set to true. As with the non-const overload,
      * it is UB to call this method if the node is not readable.
      *
@@ -377,7 +387,7 @@ struct RoNodeMethods
      *
      * Behaves similar to the non-const overload, but further asserts
      * that the returned node is readable (because it can never be in
-     * a seed state). This assertion is performed only if @ref
+     * a seed state). The assertion is performed only if @ref
      * RYML_USE_ASSERT is set to true. As with the non-const overload,
      * it is UB to use the return value if it is not valid.
      *
@@ -407,6 +417,90 @@ struct RoNodeMethods
         return {tree_, ch};
     }
 
+    /** @} */
+
+public:
+
+    /** @name at
+     *
+     * These functions are the analogue to operator[], with the
+     * difference that they */
+    /** @{ */
+
+    /** Find child by key; complexity is O(num_children).
+     *
+     * Returns the requested node, or an object in seed state if no
+     * such child is found (see @ref NodeRef for an explanation of
+     * what is seed state). When the object is in seed state, using it
+     * to read from the tree is UB. The seed node can be subsequently
+     * used to write to the tree provided that its create() method is
+     * called prior to writing, which happens inside most mutating
+     * methods in NodeRef. It is the caller's responsibility to verify
+     * that the returned node is readable before subsequently using it
+     * to read from the tree.
+     *
+     * @warning This method will call the error callback (regardless
+     * of build type) whenever any of the following preconditions is
+     * violated: a) the object is valid (points at a tree and a node),
+     * b) the calling object must be readable (must not be in seed
+     * state), c) the calling object must be pointing at a MAP
+     * node. The preconditions are similar to the non-const
+     * operator[](csubstr), but instead of using assertions, this
+     * function directly checks those conditions and calls the error
+     * callback if any of the checks fail.
+     *
+     * @note since it is valid behavior for the returned node to be in
+     * seed state, the error callback is not invoked when this
+     * happens. */
+    template<class U=Impl>
+    C4_ALWAYS_INLINE auto at(csubstr key) -> _C4_IF_MUTABLE(Impl)
+    {
+        RYML_CHECK(tree_ != nullptr);
+        _RYML_CB_CHECK(tree_->m_callbacks, (id_ >= 0 && id_ < tree_->capacity()));
+        _RYML_CB_CHECK(tree_->m_callbacks, ((Impl const*)this)->readable());
+        _RYML_CB_CHECK(tree_->m_callbacks, tree_->is_map(id_));
+        size_t ch = tree__->find_child(id__, key);
+        return ch != NONE ? Impl(tree__, ch) : Impl(tree__, id__, key);
+    }
+
+    /** Find child by position; complexity is O(pos).
+     *
+     * Returns the requested node, or an object in seed state if no
+     * such child is found (see @ref NodeRef for an explanation of
+     * what is seed state). When the object is in seed state, using it
+     * to read from the tree is UB. The seed node can be used to write
+     * to the tree provided that its create() method is called prior
+     * to writing, which happens in most modifying methods in
+     * NodeRef. It is the caller's responsibility to verify that the
+     * returned node is readable before subsequently using it to read
+     * from the tree.
+     *
+     * @warning This method will call the error callback (regardless
+     * of build type) whenever any of the following preconditions is
+     * violated: a) the object is valid (points at a tree and a node),
+     * b) the calling object must be readable (must not be in seed
+     * state), c) the calling object must be pointing at a MAP
+     * node. The preconditions are similar to the non-const
+     * operator[](size_t), but instead of using assertions, this
+     * function directly checks those conditions and calls the error
+     * callback if any of the checks fail.
+     *
+     * @note since it is valid behavior for the returned node to be in
+     * seed state, the error callback is not invoked when this
+     * happens. */
+    template<class U=Impl>
+    C4_ALWAYS_INLINE auto at(size_t pos) -> _C4_IF_MUTABLE(Impl)
+    {
+        RYML_CHECK(tree_ != nullptr);
+        const size_t cap = tree_->capacity();
+        _RYML_CB_CHECK(tree_->m_callbacks, (id_ >= 0 && id_ < cap));
+        _RYML_CB_CHECK(tree_->m_callbacks, (pos >= 0 && pos < cap));
+        _RYML_CB_CHECK(tree_->m_callbacks, ((Impl const*)this)->readable());
+        _RYML_CB_CHECK(tree_->m_callbacks, tree_->is_container(id_));
+        size_t ch = tree__->child(id__, pos);
+        return ch != NONE ? Impl(tree__, ch) : Impl(tree__, id__, pos);
+    }
+
     /** Get a child by name, with error checking; complexity is
      * O(num_children).
      *
@@ -419,8 +513,9 @@ struct RoNodeMethods
     ConstImpl at(csubstr key) const
     {
         RYML_CHECK(tree_ != nullptr);
+        _RYML_CB_CHECK(tree_->m_callbacks, (id_ >= 0 && id_ < tree_->capacity()));
         _RYML_CB_CHECK(tree_->m_callbacks, ((Impl const*)this)->readable());
-        _RYML_CB_CHECK(tree_->m_callbacks, ((Impl const*)this)->is_map());
+        _RYML_CB_CHECK(tree_->m_callbacks, tree_->is_map(id_));
         size_t ch = tree_->find_child(id_, key);
         _RYML_CB_CHECK(tree_->m_callbacks, ch != NONE);
         return {tree_, ch};
@@ -432,16 +527,19 @@ struct RoNodeMethods
      * Behaves as operator[](size_t) const, but always raises an error
      * (even when RYML_USE_ASSERT is set to false) when the returned
      * node does not exist, or when this node is not readable, or when
-     * it is not a map. This behaviour is similar to
+     * it is not a container. This behaviour is similar to
      * std::vector::at(), but the error consists in calling the error
      * callback instead of directly raising an exception. */
     ConstImpl at(size_t pos) const
     {
         RYML_CHECK(tree_ != nullptr);
+        const size_t cap = tree_->capacity();
+        _RYML_CB_CHECK(tree_->m_callbacks, (id_ >= 0 && id_ < cap));
+        _RYML_CB_CHECK(tree_->m_callbacks, (pos >= 0 && pos < cap));
         _RYML_CB_CHECK(tree_->m_callbacks, ((Impl const*)this)->readable());
-        _RYML_CB_CHECK(tree_->m_callbacks, ((Impl const*)this)->is_container());
+        _RYML_CB_CHECK(tree_->m_callbacks, tree_->is_container(id_));
         size_t ch = tree_->child(id_, pos);
-        _RYML_CB_ASSERT(tree_->m_callbacks, ch != NONE);
+        _RYML_CB_CHECK(tree_->m_callbacks, ch != NONE);
         return {tree_, ch};
     }
 
@@ -650,6 +748,13 @@ struct RoNodeMethods
 //-----------------------------------------------------------------------------
 //-----------------------------------------------------------------------------
 //-----------------------------------------------------------------------------
+/** This object holds a pointer to an existing tree, and a node id. It
+ * can be used only to read from the tree.
+ *
+ *
+ *
+ * @warning The lifetime of the tree must be larger than that of this
+ * object. It is up to the user to ensure that this happens. */
 class RYML_EXPORT ConstNodeRef : public detail::RoNodeMethods<ConstNodeRef, ConstNodeRef>
 {
 public:
@@ -746,9 +851,21 @@ class RYML_EXPORT ConstNodeRef : public detail::RoNodeMethods<ConstNodeRef, Cons
 //-----------------------------------------------------------------------------
 //-----------------------------------------------------------------------------
 
-/** a reference to a node in an existing yaml tree, offering a more
+/** A reference to a node in an existing yaml tree, offering a more
  * convenient API than the index-based API used in the tree. This
- * reference can be used to modify the tree. Individual objects may be */
+ * reference can be used to modify the tree.
+ *
+ * When the object is in seed state, using it to read from the tree is
+ * UB. The seed node can be used to write to the tree provided that
+ * its create() method is called prior to writing, which happens in
+ * most modifying methods in NodeRef. It is the owners's
+ * responsibility to verify that the an existing node is readable
+ * before subsequently using it to read from the tree.
+ *
+ * See the quickstart for a more detailed explanation on the
+ *
+ * Individual objects may be
+ * */
 class RYML_EXPORT NodeRef : public detail::RoNodeMethods<NodeRef, ConstNodeRef>
 {
 public:
@@ -1318,6 +1435,7 @@ class RYML_EXPORT NodeRef : public detail::RoNodeMethods<NodeRef, ConstNodeRef>
     /** @} */
 
 #undef _C4RV
+#undef _C4RID
 };
 
 

test/test_case.cpp

@@ -263,9 +263,9 @@ void ExpectError::check_assertion(Tree *tree, std::function<void()> fn, Location
     #if RYML_USE_ASSERT
     ExpectError::do_check(tree, fn, expected_location);
     #else
-    C4_UNUSED(tree);
-    C4_UNUSED(fn);
-    C4_UNUSED(expected_location);
+    C4_UNUSER(tree);
+    C4_UNUSER(fn);
+    C4_UNUSER(expected_location);
     #endif
 }
 

test/test_case.hpp

@@ -171,10 +171,10 @@ struct ExpectError
 
     static void do_check(            std::function<void()> fn, Location expected={}) { do_check(nullptr, fn, expected); }
     static void do_check(Tree *tree, std::function<void()> fn, Location expected={});
+    static void check_success(            std::function<void()> fn) { check_success(nullptr, fn); }
+    static void check_success(Tree *tree, std::function<void()> fn);
     static void check_assertion(            std::function<void()> fn, Location expected={}) { check_assertion(nullptr, fn, expected); }
     static void check_assertion(Tree *tree, std::function<void()> fn, Location expected={});
-    static void check_success(std::function<void()> fn) { check_success(nullptr, fn); }
-    static void check_success(Tree *tree, std::function<void()> fn);
 };