This is an archive of the discontinued LLVM Phabricator instance.

Allow SmallVector to be used as a yaml sequence
ClosedPublic

Authored by zturner on Aug 5 2016, 11:00 AM.

Details

Summary

Currently YAML sequences require std::vectors. All of the methods that the YAML parser accesses though are present in SmallVector, so there's no reason we can't support SmallVector inherently. This patch does that.

Diff Detail

Repository
rL LLVM

Event Timeline

zturner updated this revision to Diff 66979.Aug 5 2016, 11:00 AM
zturner retitled this revision from to Allow SmallVector to be used as a yaml sequence.
zturner updated this object.
zturner added reviewers: majnemer, arphaman, kledzik.
zturner added a subscriber: llvm-commits.
majnemer added inline comments.Aug 5 2016, 11:05 AM
include/llvm/Support/YAMLTraits.h
1362–1382 ↗(On Diff #66979)

Is this formatted right? it looks funny to me for some reason.

1381 ↗(On Diff #66979)

Where is flow used?

1406 ↗(On Diff #66979)

Shouldn't this be unsigned to match SmallVector's template?

1420 ↗(On Diff #66979)

Ditto.

zturner added inline comments.Aug 5 2016, 11:31 AM
include/llvm/Support/YAMLTraits.h
1362–1382 ↗(On Diff #66979)

It's clang-formatted, so I guess so. Which party looks funny?

1381 ↗(On Diff #66979)

It seems to be used to distinguish a flow sequence from a regular sequence. I don't know how it's used, other than to say that it's an optional member that if defined, causes the parser to treat it differently. See line 47 of this file.

That said, FlowSequenceTraitsImpl and SequenceTraitsImpl are identical with the exception of the existence of the flow variable. So I'm going to remove this class and consolidate. New patch incoming shortly.

1406 ↗(On Diff #66979)

Yes, good catch.

zturner updated this revision to Diff 66985.Aug 5 2016, 11:32 AM
majnemer accepted this revision.Aug 5 2016, 11:58 AM
majnemer edited edge metadata.

LGTM with nits.

include/llvm/Support/YAMLTraits.h
1362 ↗(On Diff #66985)

My clang-format would put this on its own line.

This revision is now accepted and ready to land.Aug 5 2016, 11:58 AM
This revision was automatically updated to reflect the committed changes.