[yaml] Add internal::Node class

[jwnimmer-tri]

This will be be used to avoid yaml-cpp quirks and dependency conflicts in future commits (#15866, #15867). Towards #15868.

---

This change is 

[jwnimmer-tri]

+@DamrongGuoy for feature review, please.

[xuchenhan-tri]

Reviewed 3 of 4 files at r2, all commit messages.
Reviewable status: LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy)

---

common/yaml/yaml_node.h, line 33 at r2 (raw file):

- Sequence[Node]
- Map[string, Node]
Refer to https://yaml.org/spec/1.2.2/ for details.

FYI, I found the more pin-pointed section: https://yaml.org/spec/1.2.2/#3211-nodes more useful.

[jwnimmer-tri]

Reviewable status: LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy and @xuchenhan-tri)

---

common/yaml/yaml_node.h, line 33 at r2 (raw file):

Previously, xuchenhan-tri wrote…

FYI, I found the more pin-pointed section: https://yaml.org/spec/1.2.2/#3211-nodes more useful.

Done. Thanks!

[DamrongGuoy]

The first pass is done. PTAL.

Reviewed 2 of 4 files at r2, 1 of 1 files at r3, all commit messages.
Reviewable status: 5 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @jwnimmer-tri)

---

common/yaml/yaml_node.h, line 16 at r2 (raw file):

namespace internal {

/* The three possible kinds of YAML nodes. */

Is it mentioned somewhere that it's YAML 1.2.2 ? It took me a while to figure out why all the URLs have 1.2.2. Previously I thought it meant something like Chapter 1, Section 2, Subsection 2.

---

common/yaml/yaml_node.h, line 24 at r2 (raw file):

  kSequence,

  // See https://yaml.org/spec/1.2.2/#21-collections.

Do both kMap and kSequence refer to the same YAML document#21-collections ?

---

common/yaml/yaml_node.h, line 33 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Done. Thanks!

Thanks. That #3211-nodes did help me a lot.

---

common/yaml/yaml_node.h, line 78 at r3 (raw file):

  /* Compares two nodes for equality. */
  friend bool operator==(const Node& a, const Node& b);

Is it a precondition that a and b must have the same node type? For example, I do not see a code or a test to confirm or deny something like this (please let me know if I misread the .cc or the _test.cc):

Node dut = Node::MakeScalar("foo");
Node dut2 = Node::MakeSequence();
// EXPECT_FALSE(dut == dut2);
// or DRAKE_EXPECT_THROWS_MESSAGE(dut == dut2, ...); ?

Perhaps a precondition like this would do it?

@pre `a` and `b` must have the same node type.

---

common/yaml/yaml_node.cc, line 54 at r3 (raw file):

    [](const ScalarData&) { return NodeType::kScalar; },
    [](const SequenceData&) { return NodeType::kSequence; },
    [](const MapData&) { return NodeType::kMap; },

I'm just curious to see the trailing ,.
Is that how overloaded work: overloaded{f1, f2, f3,} ?

---

common/yaml/test/yaml_node_test.cc, line 193 at r3 (raw file):

  std::optional<std::vector<Node>> sequence;
  std::optional<std::map<std::string, Node>> map;
};

I wonder whether the Rule of three ("Three strikes and you refactor") applies here for some duplication in ScalarOperations, SequenceOperations, and MapOperations? Or it's intentional to have each node type tested independently?

[jwnimmer-tri]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy)

---

common/yaml/yaml_node.h, line 16 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Is it mentioned somewhere that it's YAML 1.2.2 ? It took me a while to figure out why all the URLs have 1.2.2. Previously I thought it meant something like Chapter 1, Section 2, Subsection 2.

Done. I would have loved to cite a URL but without a specific version locked in, because we don't care about the version here, but I wasn't able to find any such link. I've clarified in the overview, instead.

---

common/yaml/yaml_node.h, line 24 at r2 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Do both kMap and kSequence refer to the same YAML document#21-collections ?

Done. I added specific numbered examples for clarity.

---

common/yaml/yaml_node.h, line 78 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Is it a precondition that a and b must have the same node type? For example, I do not see a code or a test to confirm or deny something like this (please let me know if I misread the .cc or the _test.cc):

Node dut = Node::MakeScalar("foo");
Node dut2 = Node::MakeSequence();
// EXPECT_FALSE(dut == dut2);
// or DRAKE_EXPECT_THROWS_MESSAGE(dut == dut2, ...); ?

Perhaps a precondition like this would do it?

@pre `a` and `b` must have the same node type.

Done (in the unit test). It was missing test cases, not a documentation.

---

common/yaml/yaml_node.cc, line 54 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

I'm just curious to see the trailing ,.
Is that how overloaded work: overloaded{f1, f2, f3,} ?

In C++, in a list, the trailing comma is not required and has no effect.

However, in a multi-line list, I find it nice to keep each line uniformly terminated with the comma, so that as we re-arrange the lines and/or add more cases, we don't have to play games with which one comes last.

---

common/yaml/test/yaml_node_test.cc, line 193 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

I wonder whether the Rule of three ("Three strikes and you refactor") applies here for some duplication in ScalarOperations, SequenceOperations, and MapOperations? Or it's intentional to have each node type tested independently?

We should be testing all functions against all data types, so we can't remove any EXPECT... calls. The question is whether we should do any of the EXPECT... calls in some kind of loop or reusable test fixture, instead of writing them out one by one.

Do you have ideas for parts of the tests that would be better as loops, or fixtures, or value-parameterized tests, or something similar?

So far, the only one I see is some of the DRAKE_EXPECT_THROWS_MESSAGE stuff. For example, we have these two calls for testing error messages when calling a Sequence-only function on a non-Sequence value:

GTEST_TEST(YamlNodeTest, ScalarOperations) {
...
  DRAKE_EXPECT_THROWS_MESSAGE(
      dut.Add(Node{}),
      ".*Cannot.*Add.*on a Scalar.*");
...}

GTEST_TEST(YamlNodeTest, MapOperations) {
...
  DRAKE_EXPECT_THROWS_MESSAGE(
      dut.Add(Node{}),
      ".*Cannot.*Add.*on a Map.*");
...}

Possibly we could have a reusable function to run that check (which we would call twice), though I'm not sure that's a net benefit.

Suggestions?

[DamrongGuoy]

Reviewed 2 of 2 files at r4, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy)

---

common/yaml/yaml_node.h, line 16 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Done. I would have loved to cite a URL but without a specific version locked in, because we don't care about the version here, but I wasn't able to find any such link. I've clarified in the overview, instead.

It's clear now. Thanks!

---

common/yaml/yaml_node.h, line 24 at r2 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Done. I added specific numbered examples for clarity.

I see. Thanks!

---

common/yaml/yaml_node.h, line 78 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Done (in the unit test). It was missing test cases, not a documentation.

I see. Thanks!

---

common/yaml/test/yaml_node_test.cc, line 193 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

We should be testing all functions against all data types, so we can't remove any EXPECT... calls. The question is whether we should do any of the EXPECT... calls in some kind of loop or reusable test fixture, instead of writing them out one by one.

Do you have ideas for parts of the tests that would be better as loops, or fixtures, or value-parameterized tests, or something similar?

So far, the only one I see is some of the DRAKE_EXPECT_THROWS_MESSAGE stuff. For example, we have these two calls for testing error messages when calling a Sequence-only function on a non-Sequence value:

GTEST_TEST(YamlNodeTest, ScalarOperations) {
...
  DRAKE_EXPECT_THROWS_MESSAGE(
      dut.Add(Node{}),
      ".*Cannot.*Add.*on a Scalar.*");
...}

GTEST_TEST(YamlNodeTest, MapOperations) {
...
  DRAKE_EXPECT_THROWS_MESSAGE(
      dut.Add(Node{}),
      ".*Cannot.*Add.*on a Map.*");
...}

Possibly we could have a reusable function to run that check (which we would call twice), though I'm not sure that's a net benefit.

Suggestions?

Thanks for confirming that refactoring is desirable. Let me look at the code more carefully to suggest something concretely.

[sherm1]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee DamrongGuoy, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy)

---

-- commits, line 2 at r4:
FYI although I fixed the spelling in the PR title the commit message here still says "interal" rather than "internal".

[DamrongGuoy]

with an optional change proposed in the unit test.

Reviewable status: 1 unresolved discussion, needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @jwnimmer-tri)

---

-- commits, line 2 at r4:

Previously, sherm1 (Michael Sherman) wrote…

FYI although I fixed the spelling in the PR title the commit message here still says "interal" rather than "internal".

Good catch!

---

common/yaml/test/yaml_node_test.cc, line 193 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Thanks for confirming that refactoring is desirable. Let me look at the code more carefully to suggest something concretely.

Here's is my proposal:

$ git diff
diff --git a/common/yaml/test/yaml_node_test.cc b/common/yaml/test/yaml_node_test.cc
index 94c515b56b..afb0dbbbf1 100644
--- a/common/yaml/test/yaml_node_test.cc
+++ b/common/yaml/test/yaml_node_test.cc
@@ -2,6 +2,8 @@
 
 #include <gtest/gtest.h>
 
+#include <fmt/format.h>
+
 #include "drake/common/test_utilities/expect_throws_message.h"
 
 namespace drake {
@@ -37,6 +39,72 @@ GTEST_TEST(YamlNodeTest, CopyMove) {
   EXPECT_EQ(bar.GetScalar(), "foo");
 }
 
+// Check common operations on a Scalar, Sequence or Map.
+GTEST_TEST(YamlNodeTest, CommonOperations) {
+  struct TestTable {
+    Node dut;
+    NodeType type;
+    std::string type_string;
+    bool is_scalar;
+    bool is_empty_scalar;
+    bool is_sequence;
+    bool is_map;
+  };
+  const Node scalar = Node::MakeScalar("foo");
+  const Node sequence = Node::MakeSequence();
+  const Node map = Node::MakeMap();
+  std::vector<TestTable> test_table{
+      {scalar,   NodeType::kScalar,   "Scalar",   true,  false, false, false},
+      {sequence, NodeType::kSequence, "Sequence", false, false, true,  false},
+      {map,      NodeType::kMap,      "Map",      false, false, false, true}};
+  for (size_t i = 0; i < test_table.size(); ++i) {
+    SCOPED_TRACE(fmt::format("test_table[{}]\n", i));
+    auto [dut, type, type_string, is_scalar, is_empty_scalar, is_sequence,
+                is_map] = test_table[i];
+    EXPECT_EQ(dut.GetType(), type);
+    EXPECT_EQ(dut.GetTypeString(), type_string);
+    EXPECT_EQ(Node::GetTypeString(type), type_string);
+    EXPECT_EQ(dut.IsScalar(), is_scalar);
+    EXPECT_EQ(dut.IsEmptyScalar(), is_empty_scalar);
+    EXPECT_EQ(dut.IsSequence(), is_sequence);
+    EXPECT_EQ(dut.IsMap(), is_map);
+
+    EXPECT_EQ(dut.GetTag(), "");
+    Node dut2(dut);
+    EXPECT_TRUE(dut == dut2);
+    dut2.SetTag("bar");
+    EXPECT_FALSE(dut == dut2);
+
+    if (type != NodeType::kScalar) {
+      DRAKE_EXPECT_THROWS_MESSAGE(
+          dut.GetScalar(),
+          ".*Cannot.*GetScalar.*on a " + type_string + ".*");
+    }
+    if (type != NodeType::kSequence) {
+      DRAKE_EXPECT_THROWS_MESSAGE(
+          dut.GetSequence(),
+          ".*Cannot.*GetSequence.*on a " + type_string + ".*");
+      DRAKE_EXPECT_THROWS_MESSAGE(
+          dut.Add(Node{}),
+          ".*Cannot.*Add.*on a " + type_string + ".*");
+    }
+    if (type != NodeType::kMap) {
+      DRAKE_EXPECT_THROWS_MESSAGE(
+          dut.GetMap(),
+          ".*Cannot.*GetMap.*on a " + type_string + ".*");
+      DRAKE_EXPECT_THROWS_MESSAGE(
+          dut.Add("key", Node{}),
+          ".*Cannot.*Add.*on a " + type_string + ".*");
+      DRAKE_EXPECT_THROWS_MESSAGE(
+          dut.At("key"),
+          ".*Cannot.*At.*on a " + type_string + ".*");
+      DRAKE_EXPECT_THROWS_MESSAGE(
+          dut.Remove("key"),
+          ".*Cannot.*Remove.*on a " + type_string + ".*");
+    }
+  }
+}
+
 // Check all operations on a Scalar, except visiting and copy/move.
 GTEST_TEST(YamlNodeTest, ScalarOperations) {
   Node dut = Node::MakeScalar("foo");

Please do not worry if it changes too much. I'll lgtm without this change already.

[jwnimmer-tri]

Reviewable status: needs platform reviewer assigned, needs at least two assigned reviewers (waiting on @DamrongGuoy)

---

common/yaml/test/yaml_node_test.cc, line 193 at r3 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Here's is my proposal:

$ git diff
diff --git a/common/yaml/test/yaml_node_test.cc b/common/yaml/test/yaml_node_test.cc
index 94c515b56b..afb0dbbbf1 100644
--- a/common/yaml/test/yaml_node_test.cc
+++ b/common/yaml/test/yaml_node_test.cc
@@ -2,6 +2,8 @@
 
 #include <gtest/gtest.h>
 
+#include <fmt/format.h>
+
 #include "drake/common/test_utilities/expect_throws_message.h"
 
 namespace drake {
@@ -37,6 +39,72 @@ GTEST_TEST(YamlNodeTest, CopyMove) {
   EXPECT_EQ(bar.GetScalar(), "foo");
 }
 
+// Check common operations on a Scalar, Sequence or Map.
+GTEST_TEST(YamlNodeTest, CommonOperations) {
+  struct TestTable {
+    Node dut;
+    NodeType type;
+    std::string type_string;
+    bool is_scalar;
+    bool is_empty_scalar;
+    bool is_sequence;
+    bool is_map;
+  };
+  const Node scalar = Node::MakeScalar("foo");
+  const Node sequence = Node::MakeSequence();
+  const Node map = Node::MakeMap();
+  std::vector<TestTable> test_table{
+      {scalar,   NodeType::kScalar,   "Scalar",   true,  false, false, false},
+      {sequence, NodeType::kSequence, "Sequence", false, false, true,  false},
+      {map,      NodeType::kMap,      "Map",      false, false, false, true}};
+  for (size_t i = 0; i < test_table.size(); ++i) {
+    SCOPED_TRACE(fmt::format("test_table[{}]\n", i));
+    auto [dut, type, type_string, is_scalar, is_empty_scalar, is_sequence,
+                is_map] = test_table[i];
+    EXPECT_EQ(dut.GetType(), type);
+    EXPECT_EQ(dut.GetTypeString(), type_string);
+    EXPECT_EQ(Node::GetTypeString(type), type_string);
+    EXPECT_EQ(dut.IsScalar(), is_scalar);
+    EXPECT_EQ(dut.IsEmptyScalar(), is_empty_scalar);
+    EXPECT_EQ(dut.IsSequence(), is_sequence);
+    EXPECT_EQ(dut.IsMap(), is_map);
+
+    EXPECT_EQ(dut.GetTag(), "");
+    Node dut2(dut);
+    EXPECT_TRUE(dut == dut2);
+    dut2.SetTag("bar");
+    EXPECT_FALSE(dut == dut2);
+
+    if (type != NodeType::kScalar) {
+      DRAKE_EXPECT_THROWS_MESSAGE(
+          dut.GetScalar(),
+          ".*Cannot.*GetScalar.*on a " + type_string + ".*");
+    }
+    if (type != NodeType::kSequence) {
+      DRAKE_EXPECT_THROWS_MESSAGE(
+          dut.GetSequence(),
+          ".*Cannot.*GetSequence.*on a " + type_string + ".*");
+      DRAKE_EXPECT_THROWS_MESSAGE(
+          dut.Add(Node{}),
+          ".*Cannot.*Add.*on a " + type_string + ".*");
+    }
+    if (type != NodeType::kMap) {
+      DRAKE_EXPECT_THROWS_MESSAGE(
+          dut.GetMap(),
+          ".*Cannot.*GetMap.*on a " + type_string + ".*");
+      DRAKE_EXPECT_THROWS_MESSAGE(
+          dut.Add("key", Node{}),
+          ".*Cannot.*Add.*on a " + type_string + ".*");
+      DRAKE_EXPECT_THROWS_MESSAGE(
+          dut.At("key"),
+          ".*Cannot.*At.*on a " + type_string + ".*");
+      DRAKE_EXPECT_THROWS_MESSAGE(
+          dut.Remove("key"),
+          ".*Cannot.*Remove.*on a " + type_string + ".*");
+    }
+  }
+}
+
 // Check all operations on a Scalar, except visiting and copy/move.
 GTEST_TEST(YamlNodeTest, ScalarOperations) {
   Node dut = Node::MakeScalar("foo");

Please do not worry if it changes too much. I'll lgtm without this change already.

Done.

Well, it's about +20 LOC now from the refactor, but possibly will be easier to maintain and use over time.

jwnimmer@call-cps:~/jwnimmer-tri/drake$ git diff | diffstat
 yaml_node_test.cc |  323 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 175 insertions(+), 154 deletions(-)

jwnimmer@call-cps:~/jwnimmer-tri/drake$ bazel-bin/common/yaml/yaml_node_test --gtest_list_tests
YamlNodeTest.
  DefaultCtor
  CopyMove
Suite/YamlNodeParamaterizedTest.
  GetType/0  # GetParam() = (NodeType::kScalar, "Scalar")
  GetType/1  # GetParam() = (NodeType::kSequence, "Sequence")
  GetType/2  # GetParam() = (NodeType::kMap, "Map")
  StaticTypeString/0  # GetParam() = (NodeType::kScalar, "Scalar")
  StaticTypeString/1  # GetParam() = (NodeType::kSequence, "Sequence")
  StaticTypeString/2  # GetParam() = (NodeType::kMap, "Map")
  GetSetTag/0  # GetParam() = (NodeType::kScalar, "Scalar")
  GetSetTag/1  # GetParam() = (NodeType::kSequence, "Sequence")
  GetSetTag/2  # GetParam() = (NodeType::kMap, "Map")
  EqualityPerType/0  # GetParam() = (NodeType::kScalar, "Scalar")
  EqualityPerType/1  # GetParam() = (NodeType::kSequence, "Sequence")
  EqualityPerType/2  # GetParam() = (NodeType::kMap, "Map")
  EqualityPerTag/0  # GetParam() = (NodeType::kScalar, "Scalar")
  EqualityPerTag/1  # GetParam() = (NodeType::kSequence, "Sequence")
  EqualityPerTag/2  # GetParam() = (NodeType::kMap, "Map")
  ScalarOps/0  # GetParam() = (NodeType::kScalar, "Scalar")
  ScalarOps/1  # GetParam() = (NodeType::kSequence, "Sequence")
  ScalarOps/2  # GetParam() = (NodeType::kMap, "Map")
  SequenceOps/0  # GetParam() = (NodeType::kScalar, "Scalar")
  SequenceOps/1  # GetParam() = (NodeType::kSequence, "Sequence")
  SequenceOps/2  # GetParam() = (NodeType::kMap, "Map")
  MapOps/0  # GetParam() = (NodeType::kScalar, "Scalar")
  MapOps/1  # GetParam() = (NodeType::kSequence, "Sequence")
  MapOps/2  # GetParam() = (NodeType::kMap, "Map")
  Visiting/0  # GetParam() = (NodeType::kScalar, "Scalar")
  Visiting/1  # GetParam() = (NodeType::kSequence, "Sequence")
  Visiting/2  # GetParam() = (NodeType::kMap, "Map")

[jwnimmer-tri]

+@SeanCurtis-TRI for platform review per schedule, please.

[DamrongGuoy]

Reviewed 1 of 1 files at r5, all commit messages.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @SeanCurtis-TRI)

---

common/yaml/test/yaml_node_test.cc, line 193 at r3 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Done.

Well, it's about +20 LOC now from the refactor, but possibly will be easier to maintain and use over time.

jwnimmer@call-cps:~/jwnimmer-tri/drake$ git diff | diffstat
 yaml_node_test.cc |  323 ++++++++++++++++++++++++++++--------------------------
 1 file changed, 175 insertions(+), 154 deletions(-)

jwnimmer@call-cps:~/jwnimmer-tri/drake$ bazel-bin/common/yaml/yaml_node_test --gtest_list_tests
YamlNodeTest.
  DefaultCtor
  CopyMove
Suite/YamlNodeParamaterizedTest.
  GetType/0  # GetParam() = (NodeType::kScalar, "Scalar")
  GetType/1  # GetParam() = (NodeType::kSequence, "Sequence")
  GetType/2  # GetParam() = (NodeType::kMap, "Map")
  StaticTypeString/0  # GetParam() = (NodeType::kScalar, "Scalar")
  StaticTypeString/1  # GetParam() = (NodeType::kSequence, "Sequence")
  StaticTypeString/2  # GetParam() = (NodeType::kMap, "Map")
  GetSetTag/0  # GetParam() = (NodeType::kScalar, "Scalar")
  GetSetTag/1  # GetParam() = (NodeType::kSequence, "Sequence")
  GetSetTag/2  # GetParam() = (NodeType::kMap, "Map")
  EqualityPerType/0  # GetParam() = (NodeType::kScalar, "Scalar")
  EqualityPerType/1  # GetParam() = (NodeType::kSequence, "Sequence")
  EqualityPerType/2  # GetParam() = (NodeType::kMap, "Map")
  EqualityPerTag/0  # GetParam() = (NodeType::kScalar, "Scalar")
  EqualityPerTag/1  # GetParam() = (NodeType::kSequence, "Sequence")
  EqualityPerTag/2  # GetParam() = (NodeType::kMap, "Map")
  ScalarOps/0  # GetParam() = (NodeType::kScalar, "Scalar")
  ScalarOps/1  # GetParam() = (NodeType::kSequence, "Sequence")
  ScalarOps/2  # GetParam() = (NodeType::kMap, "Map")
  SequenceOps/0  # GetParam() = (NodeType::kScalar, "Scalar")
  SequenceOps/1  # GetParam() = (NodeType::kSequence, "Sequence")
  SequenceOps/2  # GetParam() = (NodeType::kMap, "Map")
  MapOps/0  # GetParam() = (NodeType::kScalar, "Scalar")
  MapOps/1  # GetParam() = (NodeType::kSequence, "Sequence")
  MapOps/2  # GetParam() = (NodeType::kMap, "Map")
  Visiting/0  # GetParam() = (NodeType::kScalar, "Scalar")
  Visiting/1  # GetParam() = (NodeType::kSequence, "Sequence")
  Visiting/2  # GetParam() = (NodeType::kMap, "Map")

That is very cool! Thanks!

[SeanCurtis-TRI]

Lots of quibbles.

Reviewed 2 of 4 files at r2, 1 of 2 files at r4, all commit messages.
Reviewable status: 17 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri)

---

common/yaml/yaml_node.h, line 19 at r5 (raw file):

namespace internal {

/* The three possible kinds of YAML nodes.

It seems like this file contains a direct manifestation of the YAML "Node"

https://yaml.org/spec/1.2.2/#nodes

That link documents well the concept that is being captured here (and below). It's absence seems conspicuous.

---

common/yaml/yaml_node.h, line 26 at r5 (raw file):

*/
enum class NodeType {
  // See https://yaml.org/spec/1.2.2/#23-scalars.

btw It seems given the significance of the word "Scalar" in Drake, this particular overloading of the term probably deserves a bit more explicit attention. Certainly more than a link.

---

common/yaml/yaml_node.h, line 26 at r5 (raw file):
btw: The links you've provided here provide examples but not definitions. Is it your intent that this is inductive documentation, and not deductive?

See https://yaml.org/spec/1.2.2/#scalar for the definition and https://yaml.org/spec/1.2.2/#23-scalars for examples.

Similar for sequence and map.

---

common/yaml/yaml_node.h, line 32 at r5 (raw file):

  kSequence,

  // See https://yaml.org/spec/1.2.2/#21-collections at Example 2.2.

BTW Yaml apparently allows everything under the sun to be a key in the map. However, you are restricting to string values. Worth being explicit about that as it's a point of departure from the concept provided by Yaml?

---

common/yaml/yaml_node.h, line 41 at r5 (raw file):

- Sequence[Node]
- Map[string, Node]
Refer to https://yaml.org/spec/1.2.2/#3211-nodes for details.

BTW Ah...here's the missing link (curious that both spellings are supported).

---

common/yaml/yaml_node.h, line 48 at r5 (raw file):

  /* Returns a Scalar node with the given initial value. */
  static Node MakeScalar(std::string value = {});

BTW My comments above about definitions of node types and distinguishing "Scalar" were born right here. A string-valued scalar contributed to cognitive dissonance.

---

common/yaml/yaml_node.h, line 48 at r5 (raw file):

  /* Returns a Scalar node with the given initial value. */
  static Node MakeScalar(std::string value = {});

BTW I realized belatedly that this constructor is the only opportunity to set the value of the scalar node. In contrast to sequences and maps which get "built up" after node construction. In retrospect, I think that's worth emphasizing.

Essentially, based on node type we have the following semantics:

• Scalar: Largely "immutable" value (ignoring assignment).
• Sequence: only supports addition of elements.
• Map: Supports addition, removal, and modification of Node value in place (i.e., fully mutable).

---

common/yaml/yaml_node.h, line 87 at r5 (raw file):

  /* Gets this node's YAML tag.
  See https://yaml.org/spec/1.2.2/#24-tags. */
  const std::string& GetTag() const;

nit: Worth noting that this will be the empty string unless SetTag() has been explicitly invoked with a tag value.

---

common/yaml/yaml_node.h, line 91 at r5 (raw file):

  /* Sets this node's YAML tag.
  See https://yaml.org/spec/1.2.2/#24-tags. */
  void SetTag(std::string);

BTW You've got several instances where you simply omit parameter names. Any particular reason for this aggressive (seemingly erratic) optimization?

---

common/yaml/yaml_node.h, line 91 at r5 (raw file):

  /* Sets this node's YAML tag.
  See https://yaml.org/spec/1.2.2/#24-tags. */
  void SetTag(std::string);

nit: You should probably document a bit more about the caller responsibility here. They are responsible for setting a tag value that is correct and consistent.

---

common/yaml/yaml_node.h, line 112 at r5 (raw file):

  /* Appends a new node to the back of this Sequence. */
  void Add(Node);

BTW Ticky-tacky legal detail. But as you're giving access to a reference on the vector, it's worth noting that this call may invalidate any iterator the caller has (or just treating it as a promise to invalidate to be safe).

---

common/yaml/yaml_node.h, line 124 at r5 (raw file):

  const std::map<std::string, Node>& GetMap() const;

  /* Add a new node to this Map. */

nit: I'm wondering about the YAML requirement that keys are unique. Seems this should mention that in some way. In this case, it appears that attempts to provide a non-unique key value leads to a silent failure at adding the value.

---

common/yaml/yaml_node.h, line 129 at r5 (raw file):

  /* Gets an existing node from this Map.
  @throws std::exception the given key does not exist. */
  Node& At(std::string_view key);

nit: This method struck me as odd.

• I can't call it on a const Node (having to call GetMap() instead and use the map API.)
• It does allow me to take an existing (key, value) pair and modify the value Node via assignment in place.
  • I can do this via a call to "Remove()" and then "Add()". So, why the special-purpose method whose purpose isn't even mentioned?

---

common/yaml/yaml_node.h, line 160 at r5 (raw file):

  /* The argument type for Visit on a Map node .*/
  struct MapData final {
    std::map<std::string, Node> map;

BTW Given how common unordered_map is throughout Drake and given that Yaml specifically says the map is "unordered", is it worth adding a passing note on why map instead of unordered_map?

---

common/yaml/yaml_node.cc, line 17 at r5 (raw file):

// Boilerplate for std::visit.
template<class... Ts> struct overloaded : Ts... { using Ts::operator()...; };
template<class... Ts> overloaded(Ts...) -> overloaded<Ts...>;

BTW TIL "User-defined deduction guides". Thanks.

---

common/yaml/yaml_node.cc, line 120 at r5 (raw file):

const std::string& Node::GetScalar() const {
  return std::visit(overloaded {

nit; meta. As I read this, it looks like a style-guide infraction:

https://drake.mit.edu/styleguide/cppguide.html#Braced_Initializer_List_Format

No space between overloaded and {.

---

common/yaml/test/yaml_node_test.cc, line 25 at r5 (raw file):

// Check the default constructor.
GTEST_TEST(YamlNodeTest, DefaultCtor) {

BTW While one can make the argument that Ctor is a valid abbreviation (I'd expect the argument is along the "universally-known abbreviations" line). But is it worth having to make the argument for a test name?

---

common/yaml/test/yaml_node_test.cc, line 36 at r5 (raw file):

// exhaustively, because they are defaulted.
GTEST_TEST(YamlNodeTest, CopyMove) {
  Node dut = Node::MakeScalar("foo");

nit: While you've tested copy and move constructors, you haven't tested copy and move assignment.

This line doesn't count. This is clearly one of the cases for which the C++ standard has mandated copy elision (https://en.cppreference.com/w/cpp/language/copy_elision).

---

common/yaml/test/yaml_node_test.cc, line 48 at r5 (raw file):

}

// Parameterize the remainder of the tests across the three possible types.

BTW I really like the parameterized structure of this test. :)

---

common/yaml/test/yaml_node_test.cc, line 107 at r5 (raw file):

  EXPECT_EQ(dut.GetTypeString(), GetExpectedTypeString());
  EXPECT_EQ(dut.IsScalar(), GetExpectedType() == NodeType::kScalar);
  EXPECT_EQ(dut.IsEmptyScalar(), GetExpectedType() == NodeType::kScalar);

nit: Missing a test on calling IsEmptyScalar() where it is a scalar, but not empty.

---

common/yaml/test/yaml_node_test.cc, line 120 at r5 (raw file):

TEST_P(YamlNodeParamaterizedTest, GetSetTag) {
  Node dut = MakeEmptyDut();
  EXPECT_EQ(dut.GetTag(), "");

BTW This test is related to my request on the documentation of the method. You're testing that it starts empty, but haven't documented it.

[jwnimmer-tri]

Reviewable status: 4 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @DamrongGuoy, @jwnimmer-tri, @SeanCurtis-TRI, and @xuchenhan-tri)

---

common/yaml/yaml_node.h, line 19 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

It seems like this file contains a direct manifestation of the YAML "Node"

https://yaml.org/spec/1.2.2/#nodes

That link documents well the concept that is being captured here (and below). It's absence seems conspicuous.

Done.

---

common/yaml/yaml_node.h, line 26 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

btw It seems given the significance of the word "Scalar" in Drake, this particular overloading of the term probably deserves a bit more explicit attention. Certainly more than a link.

Done.

---

common/yaml/yaml_node.h, line 26 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

btw: The links you've provided here provide examples but not definitions. Is it your intent that this is inductive documentation, and not deductive?

See https://yaml.org/spec/1.2.2/#scalar for the definition and https://yaml.org/spec/1.2.2/#23-scalars for examples.

Similar for sequence and map.

Done.

---

common/yaml/yaml_node.h, line 32 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Yaml apparently allows everything under the sun to be a key in the map. However, you are restricting to string values. Worth being explicit about that as it's a point of departure from the concept provided by Yaml?

Done.

---

common/yaml/yaml_node.h, line 41 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Ah...here's the missing link (curious that both spellings are supported).

Done.

---

common/yaml/yaml_node.h, line 48 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW My comments above about definitions of node types and distinguishing "Scalar" were born right here. A string-valued scalar contributed to cognitive dissonance.

Done.

---

common/yaml/yaml_node.h, line 87 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Worth noting that this will be the empty string unless SetTag() has been explicitly invoked with a tag value.

Done (also in class overview).

---

common/yaml/yaml_node.h, line 91 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW You've got several instances where you simply omit parameter names. Any particular reason for this aggressive (seemingly erratic) optimization?

Oops, I did not intend to be inconsistent. (Some of the functions used to be inline, so the names were required.) I've tided it up now.

In my opinion, when the variable name in the header provides no value, its better to omit it -- both for brevity, and for the reduced maintenance of keeping the h and cc variable names identical.

---

common/yaml/yaml_node.h, line 91 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: You should probably document a bit more about the caller responsibility here. They are responsible for setting a tag value that is correct and consistent.

Done.

---

common/yaml/yaml_node.h, line 112 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Ticky-tacky legal detail. But as you're giving access to a reference on the vector, it's worth noting that this call may invalidate any iterator the caller has (or just treating it as a promise to invalidate to be safe).

Done.

---

common/yaml/yaml_node.h, line 124 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: I'm wondering about the YAML requirement that keys are unique. Seems this should mention that in some way. In this case, it appears that attempts to provide a non-unique key value leads to a silent failure at adding the value.

Done.

---

common/yaml/yaml_node.h, line 129 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: This method struck me as odd.

• I can't call it on a const Node (having to call GetMap() instead and use the map API.)
• It does allow me to take an existing (key, value) pair and modify the value Node via assignment in place.
  • I can do this via a call to "Remove()" and then "Add()". So, why the special-purpose method whose purpose isn't even mentioned?

Working

I'll investigate.

---

common/yaml/yaml_node.h, line 160 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW Given how common unordered_map is throughout Drake and given that Yaml specifically says the map is "unordered", is it worth adding a passing note on why map instead of unordered_map?

OK No, I don't think so. Anything that we ever iterate over needs to be ordered. I don't think that's unique to this class.

---

common/yaml/yaml_node.cc, line 17 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW TIL "User-defined deduction guides". Thanks.

(I don't actually understand it, I just cargo cult https://en.cppreference.com/w/cpp/utility/variant/visit#Example.)

---

common/yaml/yaml_node.cc, line 120 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit; meta. As I read this, it looks like a style-guide infraction:

https://drake.mit.edu/styleguide/cppguide.html#Braced_Initializer_List_Format

No space between overloaded and {.

Done.

---

common/yaml/test/yaml_node_test.cc, line 25 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW While one can make the argument that Ctor is a valid abbreviation (I'd expect the argument is along the "universally-known abbreviations" line). But is it worth having to make the argument for a test name?

Done.

---

common/yaml/test/yaml_node_test.cc, line 36 at r5 (raw file):
My thought was that this sentence justified the degree of test coverage:

We don't need to test them exhaustively, because they are defaulted.

I'm not sure that testing what std::string and std::variant do with great precision is valuable?

---

common/yaml/test/yaml_node_test.cc, line 107 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Missing a test on calling IsEmptyScalar() where it is a scalar, but not empty.

Working

Will fix.

---

common/yaml/test/yaml_node_test.cc, line 120 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW This test is related to my request on the documentation of the method. You're testing that it starts empty, but haven't documented it.

Done (documented now).

[DamrongGuoy]

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: 4 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri and @SeanCurtis-TRI)

---

common/yaml/test/yaml_node_test.cc, line 36 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

My thought was that this sentence justified the degree of test coverage:

We don't need to test them exhaustively, because they are defaulted.

I'm not sure that testing what std::string and std::variant do with great precision is valuable?

Do we have anything about this in GSG? I'm always confused about what and how to test. It scratched me a few times in the past.

[SeanCurtis-TRI]

Reviewed 3 of 3 files at r6, all commit messages.
Reviewable status: 6 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @DamrongGuoy and @jwnimmer-tri)

---

common/yaml/yaml_node.h, line 91 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Oops, I did not intend to be inconsistent. (Some of the functions used to be inline, so the names were required.) I've tided it up now.

In my opinion, when the variable name in the header provides no value, its better to omit it -- both for brevity, and for the reduced maintenance of keeping the h and cc variable names identical.

I completely subscribe to that view.

---

common/yaml/yaml_node.h, line 160 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK No, I don't think so. Anything that we ever iterate over needs to be ordered. I don't think that's unique to this class.

Strictly true, but map has an ordering that humans recognize as ordered. :) unordered_map is likewise "ordered" for the purpose of iterating, but no human, looking at the order, would infer a contract of ordering.

And taken in conjunction with the different costs of building/walking a tree vs a hash table, I was wondering if there was an active decision there as well.

I certainly imagine you'll have pretty small maps -- not large enough to care one way or the other and, even if you were to, this is a configuration act and not an inner-loop look up. So, the distinctions are probably irrelevant.

---

common/yaml/yaml_node.cc, line 17 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

(I don't actually understand it, I just cargo cult https://en.cppreference.com/w/cpp/utility/variant/visit#Example.)

I had to go searching because I couldn't make heads or tails of the syntax. I now have better insight into it. (Although, given how bad my initial understanding was, "better" is a pretty low bar.)

---

common/yaml/yaml_node.cc, line 160 at r6 (raw file):

      return data.map;
    },
    [](auto&& data) -> const std::map<std::string, Node>& {

BTW Can I just say that the fact that this works blows my mind. I wouldn't have thought that declaring a auto-valued lambda would get decomposed into multiple missing sites. Sure, variants are compile time and the visitor code apparently uses the auto to map to types. But, I thought that resolution had to be much more local. It's nice to know C++ can still be magical.

---

common/yaml/yaml_node.cc, line 173 at r6 (raw file):

      const bool inserted = result.second;
      if (!inserted) {
        const std::string& old_key = result.first->first;

BTW When I first saw this, I was non-plussed. "Why grab the "old key"? The new key was provided in the lambda?", I asked myself. Then I remembered the move. That's an easy trap to fall in.

---

common/yaml/yaml_node.cc, line 175 at r6 (raw file):

        const std::string& old_key = result.first->first;
        throw std::logic_error(fmt::format(
            "Cannot Node::Add(key, value) usign duplicate key '{}'", old_key));

nit: In the Remove error message, you are not surrounding the key value with quotes. You'll want to unify those. (I favor the quotes).

---

common/yaml/test/yaml_node_test.cc, line 36 at r5 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Do we have anything about this in GSG? I'm always confused about what and how to test. It scratched me a few times in the past.

The point I was trying to make is that you aren't even exercising them. You could have something in Node that supports copy/move constructors but invalidates the copy/move assignment and this test would be perfectly happy.

When I read "We don't need to test them exhaustively", I interpreted that as not getting into the nitty gritty of examining multiple node types, etc. I can almost accept the argument that each of the TypeData can be assumed compliant (therefore, testing one node type is sufficient). But I didn't think I should interpret it as, "we don't need to exercise all of the claimed available methods."

[jwnimmer-tri]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @DamrongGuoy, @jwnimmer-tri, and @SeanCurtis-TRI)

---

common/yaml/yaml_node.h, line 48 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW I realized belatedly that this constructor is the only opportunity to set the value of the scalar node. In contrast to sequences and maps which get "built up" after node construction. In retrospect, I think that's worth emphasizing.

Essentially, based on node type we have the following semantics:

• Scalar: Largely "immutable" value (ignoring assignment).
• Sequence: only supports addition of elements.
• Map: Supports addition, removal, and modification of Node value in place (i.e., fully mutable).

Done.

---

common/yaml/yaml_node.h, line 129 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

I'll investigate.

In the using-code (the next two PRs, linked from the top), the situation is as follows:

In cases where the user has a const map, what they are usually doing is some large an ongoing set of operations across the entire map, where they've already called GetMapping() and are using the full power of the std::map<> API to operate (conditional lookups, iteration, etc.).

In case where the user has a non-const map, it's one that they've created just the moment before, and they need to mutate it in place somehow (either by setting the tag as a kind of post-construction tidy up, or else they are move-stealing its values out from under it and then immediately discarding it).

In short, this API is useful sugar for the mutable case (where we always want to fail-fast on a missing key). For the const case, the std::map API is suitable and means that we don't need to replicate so many functions onto this class's API.

I am not sure what kind of comment to place in the code that would help clarify this. I tried to add a brief one here in case it helps.

---

common/yaml/yaml_node.h, line 160 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Strictly true, but map has an ordering that humans recognize as ordered. :) unordered_map is likewise "ordered" for the purpose of iterating, but no human, looking at the order, would infer a contract of ordering.

And taken in conjunction with the different costs of building/walking a tree vs a hash table, I was wondering if there was an active decision there as well.

I certainly imagine you'll have pretty small maps -- not large enough to care one way or the other and, even if you were to, this is a configuration act and not an inner-loop look up. So, the distinctions are probably irrelevant.

I think I was unclear in my reply. (I've also added a code comment now.)

Because unordered_map iteration order might not be consistent from one program invocation to the next, iterating over it might result in non-determinism in program behavior, which makes reproducible debugging more difficult. By default, all uses of unordered_map::begin() are a pro-forma defect. Cases where it is valid to use are an exception to the rule and require a comment justifying it.

---

common/yaml/yaml_node.cc, line 173 at r6 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW When I first saw this, I was non-plussed. "Why grab the "old key"? The new key was provided in the lambda?", I asked myself. Then I remembered the move. That's an easy trap to fall in.

Done.

---

common/yaml/yaml_node.cc, line 175 at r6 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: In the Remove error message, you are not surrounding the key value with quotes. You'll want to unify those. (I favor the quotes).

Done.

---

common/yaml/test/yaml_node_test.cc, line 107 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Working

Will fix.

Done (near line 151).

[jwnimmer-tri]

Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @DamrongGuoy and @SeanCurtis-TRI)

---

common/yaml/test/yaml_node_test.cc, line 36 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

The point I was trying to make is that you aren't even exercising them. You could have something in Node that supports copy/move constructors but invalidates the copy/move assignment and this test would be perfectly happy.

When I read "We don't need to test them exhaustively", I interpreted that as not getting into the nitty gritty of examining multiple node types, etc. I can almost accept the argument that each of the TypeData can be assumed compliant (therefore, testing one node type is sufficient). But I didn't think I should interpret it as, "we don't need to exercise all of the claimed available methods."

The DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN has a static assertion that will fail-fast in the case where copy-assignment is not implemented.

Thus we have:

• copy constructor -- tested here
• move constructor -- tested here
• copy assign -- exists per static_assert; presumed to be correct because it is defaulted
• move assign -- presumed to exist because the move constructor exists and the operators are defaulted; presumed to be correct because it is defaulted

[DamrongGuoy]

Reviewed 2 of 3 files at r7, 1 of 1 files at r8, all commit messages.
Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @SeanCurtis-TRI)

---

common/yaml/test/yaml_node_test.cc, line 36 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

The DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN has a static assertion that will fail-fast in the case where copy-assignment is not implemented.

Thus we have:

• copy constructor -- tested here
• move constructor -- tested here
• copy assign -- exists per static_assert; presumed to be correct because it is defaulted
• move assign -- presumed to exist because the move constructor exists and the operators are defaulted; presumed to be correct because it is defaulted

Would it make sense to comment on these four points explicitly in the code?

// copy constructor -- tested here
// move constructor -- tested here
// copy assign -- exists per static_assert; presumed to be correct because it is defaulted
// move assign -- presumed to exist because the move constructor exists and the operators are defaulted; presumed to be correct because it is defaulted

Or it's documented in DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN already?

[jwnimmer-tri]

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri)

---

common/yaml/test/yaml_node_test.cc, line 36 at r5 (raw file):

Previously, DamrongGuoy (Damrong Guoy) wrote…

Would it make sense to comment on these four points explicitly in the code?

// copy constructor -- tested here
// move constructor -- tested here
// copy assign -- exists per static_assert; presumed to be correct because it is defaulted
// move assign -- presumed to exist because the move constructor exists and the operators are defaulted; presumed to be correct because it is defaulted

Or it's documented in DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN already?

My dream is that it is obvious to future readers that a one-line macro only needs a one-line test, especially because DRAKE_DEFAULT... is so widely used that it should be second nature to any Drake developer. However, I think I am losing that battle, given that both reviewers here so far have expresed surprise or confusion about the idea. It suppose we should add documentation to DRAKE_DEFAULT... explaining how to test it, but I would like to do that in a separate PR.

[jwnimmer-tri]

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri)

---

common/yaml/test/yaml_node_test.cc, line 36 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

My dream is that it is obvious to future readers that a one-line macro only needs a one-line test, especially because DRAKE_DEFAULT... is so widely used that it should be second nature to any Drake developer. However, I think I am losing that battle, given that both reviewers here so far have expresed surprise or confusion about the idea. It suppose we should add documentation to DRAKE_DEFAULT... explaining how to test it, but I would like to do that in a separate PR.

OK => #15884 for that documentation. (I want to have it vetted by more of the reviewer crew than just the YAML audience here.)

[SeanCurtis-TRI]

Reviewed 1 of 3 files at r7, all commit messages.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri)

---

common/yaml/yaml_node.h, line 129 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

In the using-code (the next two PRs, linked from the top), the situation is as follows:

In cases where the user has a const map, what they are usually doing is some large an ongoing set of operations across the entire map, where they've already called GetMapping() and are using the full power of the std::map<> API to operate (conditional lookups, iteration, etc.).

In case where the user has a non-const map, it's one that they've created just the moment before, and they need to mutate it in place somehow (either by setting the tag as a kind of post-construction tidy up, or else they are move-stealing its values out from under it and then immediately discarding it).

In short, this API is useful sugar for the mutable case (where we always want to fail-fast on a missing key). For the const case, the std::map API is suitable and means that we don't need to replicate so many functions onto this class's API.

I am not sure what kind of comment to place in the code that would help clarify this. I tried to add a brief one here in case it helps.

I think this requires less of a public API-style note and more a note to other authors. I think in your effort to provide some documentation to assuage my concern, you wrote something that largely mirrors the code signature ("only available for mutable nodes" is implied by lack of const). The redirection to GetMapping() is nice though.

For me, the crux is that the intent of this design is not obvious. I imagine it would be non-obvious for future developers as well (particularly with the asymmetry in the API -- why can I mutate a map entry in place but not a sequence entry? Or a scalar?)

I'd propose that you take your observation above and distill it into something captured in the code so some future developer won't have to guess. They should be able to appreciate the problem this solves and why solve it this way. And, perhaps, they might come up with something better in C++2024.

---

common/yaml/yaml_node.h, line 160 at r5 (raw file):
I can fully accept determinism as the argument. :) You could simplify your comment simply to:

Even though YAML mappings are notionally unordered, we use an ordered map here to ensure program determinism.

---

common/yaml/test/yaml_node_test.cc, line 36 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

OK => #15884 for that documentation. (I want to have it vetted by more of the reviewer crew than just the YAML audience here.)

It seems to me that use of DRAKE_DEFAULT_COPY_AND_MOVE_AND_ASSIGN implies the author believes they're getting all four operators. Something needs to ensure that. I love the fact that we use a compile-time check (although not a static_assert) to ensure at least one of them exists. If we had such tests for all four, then no unit test would ever be required for a class that uses the macro.

It's not immediately obvious to me why only one is tested.

Could we just put a static_assert predicated on is_copy_constructible, is_copy_assignable, etc. as part of the macro?

---

common/yaml/test/yaml_node_test.cc, line 107 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

Done (near line 151).

It checks the box, although, philosophically, it really seems to belong in the GetType test. All other related calls are located there.

[jwnimmer-tri]

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @DamrongGuoy and @SeanCurtis-TRI)

---

common/yaml/yaml_node.h, line 129 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I think this requires less of a public API-style note and more a note to other authors. I think in your effort to provide some documentation to assuage my concern, you wrote something that largely mirrors the code signature ("only available for mutable nodes" is implied by lack of const). The redirection to GetMapping() is nice though.

For me, the crux is that the intent of this design is not obvious. I imagine it would be non-obvious for future developers as well (particularly with the asymmetry in the API -- why can I mutate a map entry in place but not a sequence entry? Or a scalar?)

I'd propose that you take your observation above and distill it into something captured in the code so some future developer won't have to guess. They should be able to appreciate the problem this solves and why solve it this way. And, perhaps, they might come up with something better in C++2024.

Aha, okay so as more of a design intent overview, I've removed this comment now and modified the class overview to offer an implementation note instead.

---

common/yaml/yaml_node.h, line 160 at r5 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

I can fully accept determinism as the argument. :) You could simplify your comment simply to:

Even though YAML mappings are notionally unordered, we use an ordered map here to ensure program determinism.

Done.

---

common/yaml/test/yaml_node_test.cc, line 36 at r5 (raw file):

... If we had [compile-time checks] for all four, then no unit test would ever be required ...

Consider these two signatures:
(1) Foo& operator(const Foo&);
(2) Foo& operator=(Foo&&);

A class that defines (1) but not (2) is still move assignable. We do not need an additional compile-time check for it. Taking the address of function (2) will not always compile, because some types perform move-assignment via function (1). The function (2) does not always exist.

The thing we do need a test for, in cases where we care, is that the move operation is efficient at runtime. That test is in place here.

It's not immediately obvious to me why only one is tested.

For that, I would like to address it by documenting (in #15884) why a unit test should be minimal, in cases where the developer is using DRAKE_DEFAULT....

Could we just put a static_assert predicated on is_copy_constructible, is_copy_assignable, etc. as part of the macro?

No. I am defending all future PR authors from this useless make-work. Extra tests are completely pointless. Other people look to my PRs as an example of best practices, so I do not want to insert wrong-headed tests here.

What I can offer is that whatever advice we settle on in #15884, I will post-facto rework this unit test to implement.

[DamrongGuoy]

Reviewed 1 of 1 files at r9, all commit messages.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri)

[SeanCurtis-TRI]

Reviewed 1 of 1 files at r8.
Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri)

---

common/yaml/test/yaml_node_test.cc, line 36 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

... If we had [compile-time checks] for all four, then no unit test would ever be required ...

Consider these two signatures:
(1) Foo& operator(const Foo&);
(2) Foo& operator=(Foo&&);

A class that defines (1) but not (2) is still move assignable. We do not need an additional compile-time check for it. Taking the address of function (2) will not always compile, because some types perform move-assignment via function (1). The function (2) does not always exist.

The thing we do need a test for, in cases where we care, is that the move operation is efficient at runtime. That test is in place here.

It's not immediately obvious to me why only one is tested.

For that, I would like to address it by documenting (in #15884) why a unit test should be minimal, in cases where the developer is using DRAKE_DEFAULT....

Could we just put a static_assert predicated on is_copy_constructible, is_copy_assignable, etc. as part of the macro?

No. I am defending all future PR authors from this useless make-work. Extra tests are completely pointless. Other people look to my PRs as an example of best practices, so I do not want to insert wrong-headed tests here.

What I can offer is that whatever advice we settle on in #15884, I will post-facto rework this unit test to implement.

I suspect we're not having the same conversation. Your last comment in particular suggests that your interpretation of my words isn't anywhere near my intent. I've made inferences of what I think is in your mind. However, rather than that, I'd rather show you what's in my mind and let you draw your own distinctions.

Consider the following code:

class Foo {
 public:
  Foo(const Foo&) = default;
  Foo& operator=(const Foo&) = default;
  Foo(Foo&&) = default;
  Foo& operator=(Foo&&) = default;
  
  ...
};

In my mind, such a declaration implies the following code should be well-formed:

Foo f1;
Foo f2(f1);
f1 = f2;
Foo f3(std::move(f1));
f3 = std::move(f2);

The minimum condition for this to be true is that copy construction and assignment be implemented (move semantics aren't required). The move construction and assignment get covered by the copy.

That said valid copy constructor doesn't imply valid copy assignment (e.g., a const field would permit copy construction but delete copy assignment).

The previous comment I made, which I believe you misunderstood, was toward modifying the DRAKE_DEFAULT_... macro such that the sufficient conditions are covered simply by virtue of successful compilation. No unit tests about copy/move semantics would be required at all in the typical case (the leftover case is the " yeah, but I care about moving data" case).

The DRAKE_COPYABLE_DEMAND_COPY_CAN_COMPILE static function the macro adds is insufficient.

• It doesn't cover copy assignment.
• It doesn't do anything for templated classes.
• (In my limited experiments, it didn't seem to serve as any kind of obstacle to a class that had a deleted copy constructor because its member was a limiting factor.)

My earlier, misunderstood comment was hoping that we could do some incantation with type traits inside the macro that would do this compile-time check for us. I wasn't recommending any changes to Node or its unit tests.

However, I have since tinkered and been stymied that such a thing is possible. I've now drawn the conclusion that the minimum necessary set of unit tests is:

Foo f1;
Foo f2(f1);
f1 = f2;

That would be sufficient proof that the declared defaults will lead to compilable code.

(BTW I know some non-trivial portion of this conversation would be appropriately applied to #15884. But as it relates to a point of misunderstanding, I figured I'd continue the conversation here and, if you agree with my conclusions, transfer the relevant portions there for review.)

[jwnimmer-tri]

Missing LGTM @SeanCurtis-TRI? I'm not sure if there are any more blocking items.

Reviewable status: LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri)

---

common/yaml/test/yaml_node_test.cc, line 36 at r5 (raw file):

It doesn't cover copy assignment.

Yes it does?

jwnimmer@call-cps:~/jwnimmer-tri/drake$ git diff
diff --git a/common/yaml/yaml_node.h b/common/yaml/yaml_node.h
index 505882271..6b4d83ea1 100644
--- a/common/yaml/yaml_node.h
+++ b/common/yaml/yaml_node.h
@@ -217,7 +217,7 @@ class Node final {
  private:
   using Variant = std::variant<ScalarData, SequenceData, MappingData>;
 
-  std::string tag_;
+  const std::string tag_;
   Variant data_;
 };

$ bazel test -c dbg //common/yaml/...
ERROR: common/yaml/BUILD.bazel:24:17: Compiling common/yaml/yaml_node.cc failed: (Exit 1): gcc failed: error executing command gcc ...
In file included from common/yaml/yaml_node.h:10:0,
                 from common/yaml/yaml_node.cc:1:
common/yaml/yaml_node.h: In static member function 'static void drake::yaml::internal::Node::DRAKE_COPYABLE_DEMAND_COPY_CAN_COMPILE()':
common/drake_copyable.h:65:49: error: use of deleted function 'drake::yaml::internal::Node& drake::yaml::internal::Node::operator=(const drake::yaml::internal::Node&)'
         const Classname&)>(&Classname::operator=);              \
...

It doesn't do anything for templated classes.

I haven't looked into this specifically. We can investigate as part of the documentation work in #15884.

[SeanCurtis-TRI]

Last tweak based on my chance to revisit the hot topic this morning.

Reviewed 1 of 3 files at r7, 1 of 1 files at r9, all commit messages.
Reviewable status: 2 unresolved discussions, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @jwnimmer-tri)

---

common/yaml/test/yaml_node_test.cc, line 36 at r5 (raw file):

Previously, jwnimmer-tri (Jeremy Nimmer) wrote…

It doesn't cover copy assignment.

Yes it does?

jwnimmer@call-cps:~/jwnimmer-tri/drake$ git diff
diff --git a/common/yaml/yaml_node.h b/common/yaml/yaml_node.h
index 505882271..6b4d83ea1 100644
--- a/common/yaml/yaml_node.h
+++ b/common/yaml/yaml_node.h
@@ -217,7 +217,7 @@ class Node final {
  private:
   using Variant = std::variant<ScalarData, SequenceData, MappingData>;
 
-  std::string tag_;
+  const std::string tag_;
   Variant data_;
 };

$ bazel test -c dbg //common/yaml/...
ERROR: common/yaml/BUILD.bazel:24:17: Compiling common/yaml/yaml_node.cc failed: (Exit 1): gcc failed: error executing command gcc ...
In file included from common/yaml/yaml_node.h:10:0,
                 from common/yaml/yaml_node.cc:1:
common/yaml/yaml_node.h: In static member function 'static void drake::yaml::internal::Node::DRAKE_COPYABLE_DEMAND_COPY_CAN_COMPILE()':
common/drake_copyable.h:65:49: error: use of deleted function 'drake::yaml::internal::Node& drake::yaml::internal::Node::operator=(const drake::yaml::internal::Node&)'
         const Classname&)>(&Classname::operator=);              \
...

It doesn't do anything for templated classes.

I haven't looked into this specifically. We can investigate as part of the documentation work in #15884.

Moving the conversation over to #15884.

tl;dr I reversed my copy-assign vs copy-construct reasoning. IN my mind the static method was getting the pointer to the constructor. Of course it's not. It's getting the pointer to the copy-assign. So, my assertion that it doesn't work for both stands, but my...uh..."assignment" needs to be switched.

See the other PR for more details. I'm resolving this chain in this context.

---

common/yaml/test/yaml_node_test.cc, line 35 at r9 (raw file):

// Sanity check of copy/move/assign operators.  We don't need to test them
// exhaustively, because they are defaulted.
GTEST_TEST(YamlNodeTest, CopyMove) {

nit: Let's change the test name to make it clear we're testing the application of DRAKE_DEFAULT_.... Something along the lines DefaultedCopyMove.

---

common/yaml/test/yaml_node_test.cc, line 38 at r9 (raw file):

  Node dut = Node::MakeScalar("foo");

  // Copy constructor.

btw: If we want this to serve a pedagogical purpose, I'd recommend.

// Required copy constructor test (as per DRAKE_DEFAULT_COPY_AND_MOVE_ASSIGN
// docs).

---

common/yaml/test/yaml_node_test.cc, line 43 at r9 (raw file):

  EXPECT_EQ(foo.GetScalar(), "foo");

  // Move constructor.

Per my comment in #15884, testing the move constructor is unnecessary.

[jwnimmer-tri]

Reviewable status: 1 unresolved discussion, LGTM missing from assignee SeanCurtis-TRI(platform) (waiting on @DamrongGuoy and @SeanCurtis-TRI)

---

common/yaml/test/yaml_node_test.cc, line 35 at r9 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

nit: Let's change the test name to make it clear we're testing the application of DRAKE_DEFAULT_.... Something along the lines DefaultedCopyMove.

Done.

---

common/yaml/test/yaml_node_test.cc, line 43 at r9 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Per my comment in #15884, testing the move constructor is unnecessary.

Done (via comment).

[SeanCurtis-TRI]

Reviewed 1 of 1 files at r10, all commit messages.
Reviewable status: complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),DamrongGuoy (waiting on @jwnimmer-tri)

[SeanCurtis-TRI]

Reviewable status: 1 unresolved discussion (waiting on @jwnimmer-tri)

---

common/yaml/test/yaml_node_test.cc, line 41 at r10 (raw file):

  Node foo(dut);
  EXPECT_EQ(dut.GetScalar(), "foo");
  EXPECT_EQ(foo.GetScalar(), "foo");

BTW I infer from this state, that my latest belated thought on #15884 about "always test copy assignment" won't be adopted. :)

[jwnimmer-tri]

Reviewable status: complete! all discussions resolved, LGTM from assignees SeanCurtis-TRI(platform),DamrongGuoy (waiting on @jwnimmer-tri)

---

common/yaml/test/yaml_node_test.cc, line 41 at r10 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

BTW I infer from this state, that my latest belated thought on #15884 about "always test copy assignment" won't be adopted. :)

(I didn't actually read the proposal yet, I'm still just treating this PR's choices in isolation.)

I think the suite of tests within this directory is more than sufficient to enable developers to modify Drake's YAML-related code with confidence. If there is a copy-assignment related bug, it will definitely show up as a failure somewhere.

[SeanCurtis-TRI]

---

common/yaml/test/yaml_node_test.cc, line 41 at r10 (raw file):
Ah...I still had this statement in mind:

Other people look to my PRs as an example of best practices...

and I assumed you wanted this to reflect whatever conclusions we ended up on in #15884. Now you're articulating a different priority. That's fine.