[Support] Teach YAMLIO about polymorphic types

[scott.linder]

Summary

Add support for "polymorphic" types to YAMLIO.

PolymorphicTraits can dynamically switch between other traits (Scalar, Map, or Sequence). When inputting, the PolymorphicTraits type is told which type to become, and when outputting the PolymorphicTraits type is asked which type it currently is.

Also adds support for TaggedScalarTraits to allow dynamically differentiating between multiple scalar types using YAML tags.

Some other minor changes, like serializing empty maps as "{}" and empty sequences as "[]", so that types are preserved when round-tripping PolymorphicTraits.

This patch is part of a series required for AMDGPU to support it's new MessagePack metadata.

[scott.linder]

Rebase and ping

[scott.linder]

Rebase and ping

[scott.linder]

In cleaning up this patch I noticed our YAML parser does not handle an explicit document containing only a block-level plain scalar, e.g.:

---
plain scalar
...

The YAMLTraits do not implement operator<< for the non-block scalar, presumably for this reason. As the new polymorphic trait can't determine if the value is scalar at compile-time, I added an assert to catch it dynamically for now. In the future modifying the parser to support this might be useful.

[scott.linder]

Default the test case destructor rather than give it an empty body.

[scott.linder]

Ping

[zturner]

I'm sorry this has taken so long to get a response. I'm not really sure who the best person to look at this is, but I don't think it's me unfortunately. @chandlerc , can you help find a reviewer for this?

[scott.linder]

Ping

[dblaikie]

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

include/llvm/Support/YAMLTraits.h

301 ↗

(On Diff #162860)

Perhaps this could/should be a declaration rather than a definition? (since it doesn't have any members anyway)

552 ↗

(On Diff #162860)

You can drop this since it's the default (all members are public in a struct)

lib/Support/YAMLTraits.cpp

455 ↗

(On Diff #162860)

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.

716–718 ↗

(On Diff #162860)

Any particular reason for the 'this->' qualifiers? They're probably not needed here, I think?

unittests/Support/YAMLIOTest.cpp

2663 ↗

(On Diff #162860)

I probably wouldn't typedef this - it obscures the fact that this has memory ownership semantics.

2686 ↗

(On Diff #162860)

You can drop the "public" keywords here - they're implied/the default for structs. (similarly below)

2704–2705 ↗

(On Diff #162860)

Drop "else after return" (similarly below/etc)

2714 ↗

(On Diff #162860)

Probably prefer N = llvm::make_unique<Scalar>(); here and elsewhere/in similar contexts.

2805 ↗

(On Diff #162860)

auto node = llvm::make_unique<Scalar>(true);

(& similarly elsewhere)

[scott.linder]

I can submit a separate patch to clean up things like making the empty definitions into declarations, removing redundant public: qualifiers, and removing redundant references to this, but I don't want to mix those changes into this patch.

I will update this patch with the other changes, as well as full context.

include/llvm/Support/YAMLTraits.h

301 ↗

(On Diff #162860)

I agree, but I am just following the convention in the file, which is an empty definition (see e.g. MappingContextTraits). I mistakenly uploaded the patch without context, but I will include it in the next revision.

552 ↗

(On Diff #162860)

Again I'm trying to follow a convention found throughout the file for has_ structs, although all of these could be updated to drop the redundant qualifier.

[scott.linder]

Address feedback

[scott.linder]

Cleanup patch is at https://reviews.llvm.org/D53088

[scott.linder]

Rebase and remove redundant qualifiers

[scott.linder]

Ping

[scott.linder]

Ping

[nhaehnle]

One small nitpick, apart from that LGTM.

include/llvm/Support/YAMLTraits.h

205 ↗

(On Diff #169060)

Value instead of Val, I assume.

[scott.linder]

Address feedback

[ruiu]

This is not a comment to your code (so please commit if you already got an LGTM), but I have an experience of using YAMLIO in lld and found that the library is hard to use, at least for me, perhaps due to too much uses of template. YAML is intended to be designed as a language that is easy to parse, but if you use YAMLIO, it is sometimes very tricky to write code to parse/generate a YAML file. I feel like there's a better way of doing it, and the YAMLIO library could be improved drastically. Not sure if it is worth doing, given that we don't use YAMLIO that much in LLVM, but I'd like to share my thoughts...

[scott.linder]

The heavy use of templates does lead to some awkward usage, but it can also make some common use-cases very natural. I agree there is probably room to improve, but it is orthogonal to this patch.

If there are no objections I will be applying this patch (and the updates to some Clang tests at https://reviews.llvm.org/D51223) today.

unittests/Tooling/DiagnosticsYamlTest.cpp

EXPECT_EQ("---\n"
             "      - FilePath:        'path/to/header.h'\n"
             "        Offset:          62\n"
             "        Length:          2\n"
             "        ReplacementText: 'replacement #2'\n"
             "  - DiagnosticName:  'diagnostic#3'\n"
             "    Message:         'message #3'\n"
             "    FileOffset:      72\n"
             "    FilePath:        'path/to/source2.cpp'\n"
-            "    Replacements:    \n"
+            "    Replacements:    []\n"
             "...\n",
             YamlContentStream.str());
 }
 
 TEST(DiagnosticsYamlTest, deserializesDiagnostics) {
   std::string YamlContent = "---\n"
                             "MainSourceFile:  path/to/source.cpp\n"
                             "Diagnostics:     \n"
std::string YamlContent = "---\n"
                             "      - FilePath:        path/to/header.h\n"
                             "        Offset:          62\n"
                             "        Length:          2\n"
                             "        ReplacementText: 'replacement #2'\n"
                             "  - DiagnosticName:  'diagnostic#3'\n"
                             "    Message:         'message #3'\n"
                             "    FileOffset:      98\n"
                             "    FilePath:        path/to/source.cpp\n"
-                            "    Replacements:    \n"
+                            "    Replacements:    []\n"
                             "...\n";
   TranslationUnitDiagnostics TUDActual;
   yaml::Input YAML(YamlContent);
   YAML >> TUDActual;
 
   ASSERT_FALSE(YAML.error());
   ASSERT_EQ(3u, TUDActual.Diagnostics.size());
   EXPECT_EQ("path/to/source.cpp", TUDActual.MainSourceFile);

unittests/Tooling/RefactoringActionRulesTest.cpp

// When the requirements are satisfied, the rule's function must be invoked.
     ASSERT_EQ(Result.size(), 1u);
     std::string YAMLString =
         const_cast<AtomicChange &>(Result[0]).toYAMLString();
 
     ASSERT_STREQ("---\n"
                  "Key:             'input.cpp:30'\n"
                  "FilePath:        input.cpp\n"
                  "Error:           ''\n"
-                 "InsertedHeaders: \n"
-                 "RemovedHeaders:  \n"
+                 "InsertedHeaders: []\n"
+                 "RemovedHeaders:  []\n"
                  "Replacements:    \n" // Extra whitespace here!
                  "  - FilePath:        input.cpp\n"
                  "    Offset:          30\n"
                  "    Length:          1\n"
                  "    ReplacementText: b\n"
                  "...\n",
                  YAMLString.c_str());
   }