[PATCH] D38039: [Support/YAMLTraits] - Refactor of Input::createHNodes

[George Rimar via Phabricator via llvm-commits]

grimar created this revision.

I tried to fix crash issue relative to this method and found it is hard to read.
It contained duplicated piece of code which is now moved to helper function and shared.
When error happened it returned half-broken instance of node, and since
`createHNodes` itself a reqursive method, it had to check EC in caller, 
when I believe it is enough just to return nullptr on any error, 
just like it already do for unknown node.
It is also was possible to use early returns, patch did that.


https://reviews.llvm.org/D38039

Files:
  lib/Support/YAMLTraits.cpp


Index: lib/Support/YAMLTraits.cpp
===================================================================
--- lib/Support/YAMLTraits.cpp
+++ lib/Support/YAMLTraits.cpp
@@ -349,54 +349,57 @@
   EC = make_error_code(errc::invalid_argument);
 }
 
-std::unique_ptr<Input::HNode> Input::createHNodes(Node *N) {
+static StringRef getScalarNodeValue(ScalarNode *S, BumpPtrAllocator &Alloc) {
   SmallString<128> StringStorage;
-  if (ScalarNode *SN = dyn_cast<ScalarNode>(N)) {
-    StringRef KeyStr = SN->getValue(StringStorage);
-    if (!StringStorage.empty()) {
-      // Copy string to permanent storage
-      KeyStr = StringStorage.str().copy(StringAllocator);
-    }
-    return llvm::make_unique<ScalarHNode>(N, KeyStr);
-  } else if (BlockScalarNode *BSN = dyn_cast<BlockScalarNode>(N)) {
-    StringRef ValueCopy = BSN->getValue().copy(StringAllocator);
-    return llvm::make_unique<ScalarHNode>(N, ValueCopy);
-  } else if (SequenceNode *SQ = dyn_cast<SequenceNode>(N)) {
+  StringRef KeyStr = S->getValue(StringStorage);
+  // Copy string to permanent storage
+  if (!StringStorage.empty())
+    return StringStorage.str().copy(Alloc);
+  return KeyStr;
+}
+
+std::unique_ptr<Input::HNode> Input::createHNodes(Node *N) {
+  if (ScalarNode *SN = dyn_cast<ScalarNode>(N))
+    return llvm::make_unique<ScalarHNode>(
+        SN, getScalarNodeValue(SN, StringAllocator));
+
+  if (BlockScalarNode *BSN = dyn_cast<BlockScalarNode>(N))
+    return llvm::make_unique<ScalarHNode>(
+        N, BSN->getValue().copy(StringAllocator));
+
+  if (SequenceNode *SQ = dyn_cast<SequenceNode>(N)) {
     auto SQHNode = llvm::make_unique<SequenceHNode>(N);
     for (Node &SN : *SQ) {
       auto Entry = this->createHNodes(&SN);
-      if (EC)
-        break;
+      if (!Entry)
+        return nullptr;
       SQHNode->Entries.push_back(std::move(Entry));
     }
     return std::move(SQHNode);
-  } else if (MappingNode *Map = dyn_cast<MappingNode>(N)) {
-    auto mapHNode = llvm::make_unique<MapHNode>(N);
+  }
+
+  if (MappingNode *Map = dyn_cast<MappingNode>(N)) {
+    auto MapNode = llvm::make_unique<MapHNode>(N);
     for (KeyValueNode &KVN : *Map) {
-      Node *KeyNode = KVN.getKey();
-      ScalarNode *KeyScalar = dyn_cast<ScalarNode>(KeyNode);
+      ScalarNode *KeyScalar = dyn_cast<ScalarNode>(KVN.getKey());
       if (!KeyScalar) {
-        setError(KeyNode, "Map key must be a scalar");
-        break;
-      }
-      StringStorage.clear();
-      StringRef KeyStr = KeyScalar->getValue(StringStorage);
-      if (!StringStorage.empty()) {
-        // Copy string to permanent storage
-        KeyStr = StringStorage.str().copy(StringAllocator);
+        setError(KVN.getKey(), "Map key must be a scalar");
+        return nullptr;
       }
       auto ValueHNode = this->createHNodes(KVN.getValue());
-      if (EC)
-        break;
-      mapHNode->Mapping[KeyStr] = std::move(ValueHNode);
+      if (!ValueHNode)
+        return nullptr;
+      StringRef Key = getScalarNodeValue(KeyScalar, StringAllocator);
+      MapNode->Mapping[Key] = std::move(ValueHNode);
     }
-    return std::move(mapHNode);
-  } else if (isa<NullNode>(N)) {
-    return llvm::make_unique<EmptyHNode>(N);
-  } else {
-    setError(N, "unknown node kind");
-    return nullptr;
+    return std::move(MapNode);
   }
+
+  if (isa<NullNode>(N))
+    return llvm::make_unique<EmptyHNode>(N);
+
+  setError(N, "unknown node kind");
+  return nullptr;
 }
 
 void Input::setError(const Twine &Message) {


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D38039.115843.patch
Type: text/x-patch
Size: 3486 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170919/ecec1481/attachment.bin>