Code review comments, more parser tests.

Author: marcalff

sdk/include/opentelemetry/sdk/configuration/document_node.h

@@ -105,17 +105,24 @@ class PropertiesNodeConstIteratorImpl
 class DocumentNodeConstIterator
 {
 public:
-  DocumentNodeConstIterator(DocumentNodeConstIteratorImpl *impl) : impl_(impl) {}
-  DocumentNodeConstIterator(DocumentNodeConstIterator &&)                      = default;
-  DocumentNodeConstIterator(const DocumentNodeConstIterator &)                 = default;
-  DocumentNodeConstIterator &operator=(DocumentNodeConstIterator &&)           = default;
-  DocumentNodeConstIterator &operator=(const DocumentNodeConstIterator &other) = default;
-
-  ~DocumentNodeConstIterator() { delete impl_; }
-
-  bool operator==(const DocumentNodeConstIterator &rhs) const { return (impl_->Equal(rhs.impl_)); }
+  DocumentNodeConstIterator(std::unique_ptr<DocumentNodeConstIteratorImpl> impl)
+      : impl_(std::move(impl))
+  {}
+  DocumentNodeConstIterator(DocumentNodeConstIterator &&)                      = delete;
+  DocumentNodeConstIterator(const DocumentNodeConstIterator &)                 = delete;
+  DocumentNodeConstIterator &operator=(DocumentNodeConstIterator &&)           = delete;
+  DocumentNodeConstIterator &operator=(const DocumentNodeConstIterator &other) = delete;
+  ~DocumentNodeConstIterator()                                                 = default;
+
+  bool operator==(const DocumentNodeConstIterator &rhs) const
+  {
+    return (impl_->Equal(rhs.impl_.get()));
+  }
 
-  bool operator!=(const DocumentNodeConstIterator &rhs) const { return (!impl_->Equal(rhs.impl_)); }
+  bool operator!=(const DocumentNodeConstIterator &rhs) const
+  {
+    return (!impl_->Equal(rhs.impl_.get()));
+  }
 
   std::unique_ptr<DocumentNode> operator*() const { return impl_->Item(); }
 
@@ -126,27 +133,29 @@ class DocumentNodeConstIterator
   }
 
 private:
-  DocumentNodeConstIteratorImpl *impl_;
+  std::unique_ptr<DocumentNodeConstIteratorImpl> impl_;
 };
 
 class PropertiesNodeConstIterator
 {
 public:
-  PropertiesNodeConstIterator(PropertiesNodeConstIteratorImpl *impl) : impl_(impl) {}
-  PropertiesNodeConstIterator(PropertiesNodeConstIterator &&)                      = default;
-  PropertiesNodeConstIterator(const PropertiesNodeConstIterator &)                 = default;
-  PropertiesNodeConstIterator &operator=(PropertiesNodeConstIterator &&)           = default;
-  PropertiesNodeConstIterator &operator=(const PropertiesNodeConstIterator &other) = default;
-  ~PropertiesNodeConstIterator() { delete impl_; }
+  PropertiesNodeConstIterator(std::unique_ptr<PropertiesNodeConstIteratorImpl> impl)
+      : impl_(std::move(impl))
+  {}
+  PropertiesNodeConstIterator(PropertiesNodeConstIterator &&)                      = delete;
+  PropertiesNodeConstIterator(const PropertiesNodeConstIterator &)                 = delete;
+  PropertiesNodeConstIterator &operator=(PropertiesNodeConstIterator &&)           = delete;
+  PropertiesNodeConstIterator &operator=(const PropertiesNodeConstIterator &other) = delete;
+  ~PropertiesNodeConstIterator()                                                   = default;
 
   bool operator==(const PropertiesNodeConstIterator &rhs) const
   {
-    return (impl_->Equal(rhs.impl_));
+    return (impl_->Equal(rhs.impl_.get()));
   }
 
   bool operator!=(const PropertiesNodeConstIterator &rhs) const
   {
-    return (!impl_->Equal(rhs.impl_));
+    return (!impl_->Equal(rhs.impl_.get()));
   }
 
   std::string Name() const { return impl_->Name(); }
@@ -159,7 +168,7 @@ class PropertiesNodeConstIterator
   }
 
 private:
-  PropertiesNodeConstIteratorImpl *impl_;
+  std::unique_ptr<PropertiesNodeConstIteratorImpl> impl_;
 };
 
 }  // namespace configuration

sdk/include/opentelemetry/sdk/configuration/instrument_type.h

@@ -17,6 +17,7 @@ namespace configuration
 // YAML-NODE: InstrumentType
 enum class InstrumentType : std::uint8_t
 {
+  none, /* Represents a null entry */
   counter,
   histogram,
   observable_counter,

sdk/src/configuration/configuration_parser.cc

@@ -771,6 +771,11 @@ static std::unique_ptr<MetricReaderConfiguration> ParseMetricReaderConfiguration
 
 static InstrumentType ParseInstrumentType(const std::string &name)
 {
+  if (name == "")
+  {
+    return InstrumentType::none;
+  }
+
   if (name == "counter")
   {
     return InstrumentType::counter;

sdk/src/configuration/ryml_document_node.cc

@@ -363,15 +363,20 @@ DocumentNodeConstIterator RymlDocumentNode::begin() const
   }
 #endif  // WITH_DEBUG_NODE
 
-  return DocumentNodeConstIterator(new RymlDocumentNodeConstIteratorImpl(node_, 0, depth_));
+  auto impl = std::unique_ptr<DocumentNodeConstIteratorImpl>(
+      new RymlDocumentNodeConstIteratorImpl(node_, 0, depth_));
+
+  return DocumentNodeConstIterator(std::move(impl));
 }
 
 DocumentNodeConstIterator RymlDocumentNode::end() const
 {
   OTEL_INTERNAL_LOG_DEBUG("RymlDocumentNode::end()");
 
-  return DocumentNodeConstIterator(
+  auto impl = std::unique_ptr<DocumentNodeConstIteratorImpl>(
       new RymlDocumentNodeConstIteratorImpl(node_, node_.num_children(), depth_));
+
+  return DocumentNodeConstIterator(std::move(impl));
 }
 
 size_t RymlDocumentNode::num_children() const
@@ -400,15 +405,20 @@ PropertiesNodeConstIterator RymlDocumentNode::begin_properties() const
   }
 #endif  // WITH_DEBUG_NODE
 
-  return PropertiesNodeConstIterator(new RymlPropertiesNodeConstIteratorImpl(node_, 0, depth_));
+  auto impl = std::unique_ptr<PropertiesNodeConstIteratorImpl>(
+      new RymlPropertiesNodeConstIteratorImpl(node_, 0, depth_));
+
+  return PropertiesNodeConstIterator(std::move(impl));
 }
 
 PropertiesNodeConstIterator RymlDocumentNode::end_properties() const
 {
   OTEL_INTERNAL_LOG_DEBUG("RymlDocumentNode::end_properties()");
 
-  return PropertiesNodeConstIterator(
+  auto impl = std::unique_ptr<PropertiesNodeConstIteratorImpl>(
       new RymlPropertiesNodeConstIteratorImpl(node_, node_.num_children(), depth_));
+
+  return PropertiesNodeConstIterator(std::move(impl));
 }
 
 RymlDocumentNodeConstIteratorImpl::RymlDocumentNodeConstIteratorImpl(ryml::ConstNodeRef parent,

sdk/src/configuration/yaml_configuration_parser.cc

@@ -57,22 +57,13 @@ std::unique_ptr<Configuration> YamlConfigurationParser::ParseFile(const std::str
   return conf;
 }
 
-static std::unique_ptr<Document> RymlParse(const std::string &source, const std::string &content)
-{
-  std::unique_ptr<Document> doc;
-
-  doc = RymlDocument::Parse(source, content);
-
-  return doc;
-}
-
 std::unique_ptr<Configuration> YamlConfigurationParser::ParseString(const std::string &source,
                                                                     const std::string &content)
 {
   std::unique_ptr<Document> doc;
   std::unique_ptr<Configuration> config;
 
-  doc = RymlParse(source, content);
+  doc = RymlDocument::Parse(source, content);
 
   try
   {