Page MenuHomePhabricator

[Support] Teach YAMLIO about polymorphic types
ClosedPublic

Authored by scott.linder on Jun 13 2018, 12:50 PM.

Details

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.

Diff Detail

Repository
rC Clang

Event Timeline

scott.linder created this revision.Jun 13 2018, 12:50 PM
scott.linder added a subscriber: t-tye.

Rebase and ping

Rebase and ping

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.

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

zturner resigned from this revision.Sep 13 2018, 9:51 AM

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?

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 marked 7 inline comments as done.Oct 9 2018, 12:34 PM

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.

Address feedback

Rebase and remove redundant qualifiers

nhaehnle accepted this revision.Oct 31 2018, 3:20 AM

One small nitpick, apart from that LGTM.

include/llvm/Support/YAMLTraits.h
205 ↗(On Diff #169060)

Value instead of Val, I assume.

This revision is now accepted and ready to land.Oct 31 2018, 3:20 AM

Address feedback

ruiu added a subscriber: ruiu.Oct 31 2018, 10:01 AM

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...

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.

This revision was automatically updated to reflect the committed changes.