[PATCH] D48144: [Support] Teach YAMLIO about polymorphic types

[David Blaikie via Phabricator via llvm-commits]

dblaikie added a comment.

I really don't know much about the YAMLIO code, but here's some high level review...



================
Comment at: include/llvm/Support/YAMLTraits.h:301
+///    };
+template <typename T> struct PolymorphicTraits {
+  // Must provide:
----------------
Perhaps this could/should be a declaration rather than a definition? (since it doesn't have any members anyway)


================
Comment at: include/llvm/Support/YAMLTraits.h:552
+
+public:
+  static bool const value = (sizeof(test<PolymorphicTraits<T>>(nullptr)) == 1);
----------------
You can drop this since it's the default (all members are public in a struct)


================
Comment at: lib/Support/YAMLTraits.cpp:455
+        StateStack.size() > 1 &&
+        (StateStack[StateStack.size() - 2] == inSeqFirstElement ||
+         StateStack[StateStack.size() - 2] == inSeqOtherElement ||
----------------
Maybe have a temp for the StateStack element:

  bool SequenceElement = StateStack.size() > 1;
  if (SequenceElement) {
    auto &E = StateStack[StateStack.size() - 2];
    Sequence = E == inXXX || E = inYYY ....;
  }

Or a quick utility function written somewhere , where you pass in StateStack[StateStack.size() - 2] (thus naming it, etc).

Some of the other conditions further down in the review seem similarly a bit verbose and repetitive.


================
Comment at: lib/Support/YAMLTraits.cpp:716-718
+  this->newLineCheck();
+  this->output(Tag);
+  this->output(" ");
----------------
Any particular reason for the 'this->' qualifiers? They're probably not needed here, I think?


================
Comment at: unittests/Support/YAMLIOTest.cpp:2663
+
+using PolyPtr = std::unique_ptr<Poly>;
+
----------------
I probably wouldn't typedef this - it obscures the fact that this has memory ownership semantics.


================
Comment at: unittests/Support/YAMLIOTest.cpp:2686
+
+struct Seq : public Poly, public std::vector<PolyPtr> {
+  Seq() : Poly(NK_Seq) {}
----------------
You can drop the "public" keywords here - they're implied/the default for structs. (similarly below)


================
Comment at: unittests/Support/YAMLIOTest.cpp:2704-2705
+    if (isa<Scalar>(*N))
+      return NodeKind::Scalar;
+    else if (isa<Seq>(*N))
+      return NodeKind::Sequence;
----------------
Drop "else after return" (similarly below/etc)


================
Comment at: unittests/Support/YAMLIOTest.cpp:2714
+    if (!N || !isa<Scalar>(*N))
+      N.reset(new Scalar());
+    return *cast<Scalar>(N.get());
----------------
Probably prefer N = llvm::make_unique<Scalar>(); here and elsewhere/in similar contexts.


================
Comment at: unittests/Support/YAMLIOTest.cpp:2805
+  std::string intermediate;
+  PolyPtr node = PolyPtr(new Scalar(true));
+
----------------
  auto node = llvm::make_unique<Scalar>(true);

(& similarly elsewhere)


https://reviews.llvm.org/D48144