[PATCH] D24162: [yaml] Add the ability to pass in context at the map level

[Chandler Carruth via llvm-commits]

chandlerc added a comment.

I guess I can look. I was hoping someone with more familiarity with the code would, but unless Alex shows back up, that seems unlikely. And I understand you need to get unblocked.

I find the design pattern here pretty deeply confusing. But I don't think that it is at all related to your patch, but to my lack of familiarity with YAMLIO overall. Your patch does seem to fit the existing design patterns.

Since you might be able to, have you checked if MSVC 2013 is happy with the SFINAE trait you're using? I've had trouble getting those past it in the past.

Also a minor comment on the test below.


================
Comment at: unittests/Support/YAMLIOTest.cpp:2305-2317
@@ +2304,15 @@
+namespace yaml {
+struct MappingContext {
+  int A = 0;
+};
+struct SimpleMap {
+  int B = 0;
+  int C = 0;
+};
+
+struct NestedMap {
+  NestedMap(MappingContext &Context) : Context(Context) {}
+  SimpleMap Simple;
+  MappingContext &Context;
+};
+
----------------
Shouldn't these be outside the llvm::yaml namespace and in the test's anonymous namespace?

Above, only the template specializations are in the llvm::yaml namespace which makes more sense to me.


https://reviews.llvm.org/D24162