[PATCH] YAML: Add an optional 'flow' field to the mapping trait to allow flow mapping output.
Alex L
arphaman at gmail.com
Fri May 1 18:33:42 PDT 2015
2015-05-01 17:21 GMT-07:00 Justin Bogner <mail at justinbogner.com>:
> Alex Lorenz <arphaman at gmail.com> writes:
> > Hi bogner, bob.wilson,
> >
> > This patch adds an optional flow field to the MappingTrait class that
> > enables yaml IO to output flow mappings.
> > I have a patch that updates the documentation as well, I will post it
> later.
>
> Please add the documentation in the same commit as the change.
>
> > REPOSITORY
> > rL LLVM
> >
> > http://reviews.llvm.org/D9450
> >
> > Files:
> > include/llvm/Support/YAMLTraits.h
> > lib/Support/YAMLTraits.cpp
> > unittests/Support/YAMLIOTest.cpp
> >
> > EMAIL PREFERENCES
> > http://reviews.llvm.org/settings/panel/emailpreferences/
> >
> > Index: include/llvm/Support/YAMLTraits.h
> > ===================================================================
> > --- include/llvm/Support/YAMLTraits.h
> > +++ include/llvm/Support/YAMLTraits.h
> > @@ -46,6 +46,10 @@
> > // static void mapping(IO &io, T &fields);
> > // Optionally may provide:
> > // static StringRef validate(IO &io, T &fields);
> > + //
> > + // The optional flow flag will cause generated YAML to use a flow
> mapping
> > + // (e.g. { a: 0, b: 1 }):
> > + // static const bool flow = true;
> > };
> >
> >
> > @@ -445,6 +449,9 @@
> > virtual bool preflightKey(const char*, bool, bool, bool &, void *&) =
> 0;
> > virtual void postflightKey(void*) = 0;
> >
> > + virtual void beginFlowMapping() = 0;
> > + virtual void endFlowMapping() = 0;
> > +
> > virtual void beginEnumScalar() = 0;
> > virtual bool matchEnumScalar(const char*, bool) = 0;
> > virtual bool matchEnumFallback() = 0;
> > @@ -643,7 +650,10 @@
> > template<typename T>
> > typename std::enable_if<validatedMappingTraits<T>::value, void>::type
> > yamlize(IO &io, T &Val, bool) {
> > - io.beginMapping();
> > + if (has_FlowTraits<MappingTraits<T>>::value)
> > + io.beginFlowMapping();
> > + else
> > + io.beginMapping();
> > if (io.outputting()) {
> > StringRef Err = MappingTraits<T>::validate(io, Val);
> > if (!Err.empty()) {
> > @@ -657,15 +667,24 @@
> > if (!Err.empty())
> > io.setError(Err);
> > }
> > - io.endMapping();
> > + if (has_FlowTraits<MappingTraits<T>>::value)
> > + io.endFlowMapping();
> > + else
> > + io.endMapping();
> > }
> >
> > template<typename T>
> > typename std::enable_if<unvalidatedMappingTraits<T>::value, void>::type
> > yamlize(IO &io, T &Val, bool) {
> > - io.beginMapping();
> > - MappingTraits<T>::mapping(io, Val);
> > - io.endMapping();
> > + if (has_FlowTraits<MappingTraits<T>>::value) {
> > + io.beginFlowMapping();
> > + MappingTraits<T>::mapping(io, Val);
> > + io.endFlowMapping();
> > + } else {
> > + io.beginMapping();
> > + MappingTraits<T>::mapping(io, Val);
> > + io.endMapping();
> > + }
>
> I'm not a big fan of the "check the flow traits at all of the callers"
> style that's prevalent here, but it seems to be consistent with the
> existing code for FlowElements so I guess it makes sense to do it this
> way. More below.
>
> > }
> >
> > template<typename T>
> > @@ -900,6 +919,8 @@
> > void endMapping() override;
> > bool preflightKey(const char *, bool, bool, bool &, void *&) override;
> > void postflightKey(void *) override;
> > + void beginFlowMapping() override;
> > + void endFlowMapping() override;
> > unsigned beginSequence() override;
> > void endSequence() override;
> > bool preflightElement(unsigned index, void *&) override;
> > @@ -1028,6 +1049,8 @@
> > void endMapping() override;
> > bool preflightKey(const char *key, bool, bool, bool &, void *&)
> override;
> > void postflightKey(void *) override;
> > + void beginFlowMapping() override;
> > + void endFlowMapping() override;
> > unsigned beginSequence() override;
> > void endSequence() override;
> > bool preflightElement(unsigned, void *&) override;
> > @@ -1061,12 +1084,20 @@
> > void outputNewLine();
> > void paddedKey(StringRef key);
> >
> > - enum InState { inSeq, inFlowSeq, inMapFirstKey, inMapOtherKey };
> > + enum InState {
> > + inSeq,
> > + inFlowSeq,
> > + inMapFirstKey,
> > + inMapOtherKey,
> > + inFlowMapFirstKey,
> > + inFlowMapOtherKey
> > + };
> >
> > llvm::raw_ostream &Out;
> > SmallVector<InState, 8> StateStack;
> > int Column;
> > int ColumnAtFlowStart;
> > + int ColumnAtMapFlowStart;
> > bool NeedBitValueComma;
> > bool NeedFlowSequenceComma;
> > bool EnumerationMatchFound;
> > Index: lib/Support/YAMLTraits.cpp
> > ===================================================================
> > --- lib/Support/YAMLTraits.cpp
> > +++ lib/Support/YAMLTraits.cpp
> > @@ -168,6 +168,10 @@
> > }
> > }
> >
> > +void Input::beginFlowMapping() { beginMapping(); }
> > +
> > +void Input::endFlowMapping() { endMapping(); }
> > +
> > unsigned Input::beginSequence() {
> > if (SequenceHNode *SQ = dyn_cast<SequenceHNode>(CurrentNode))
> > return SQ->Entries.size();
> > @@ -393,6 +397,7 @@
> > Out(yout),
> > Column(0),
> > ColumnAtFlowStart(0),
> > + ColumnAtMapFlowStart(0),
> > NeedBitValueComma(false),
> > NeedFlowSequenceComma(false),
> > EnumerationMatchFound(false),
> > @@ -427,8 +432,23 @@
> > bool &UseDefault, void *&) {
> > UseDefault = false;
> > if (Required || !SameAsDefault) {
> > - this->newLineCheck();
> > - this->paddedKey(Key);
> > + auto State = StateStack.back();
> > + if (State == inFlowMapFirstKey || State == inFlowMapOtherKey) {
> > + if (State == inFlowMapOtherKey)
> > + output(", ");
> > + if (Column > 70) {
> > + output("\n");
> > + for (int i = 0; i < ColumnAtMapFlowStart; ++i)
> > + output(" ");
> > + Column = ColumnAtMapFlowStart;
> > + output(" ");
> > + }
> > + output(Key);
> > + output(": ");
> > + } else {
> > + this->newLineCheck();
> > + this->paddedKey(Key);
> > + }
>
> So to be consistent this should really be split into preflightKey and
> preflightFlowKey, much like is done for elements. As it is here, there's
> a big difference in abstraction level between the two branches of the
> if, which makes this hard to follow.
>
I can extract the preflightFlowKey into a separate private method that's
called by preflightKey,
but it wouldn't be necessarily consistent I think as this won't be another
virtual method from
IO like the others and it would also have no postflightFlowKey sibling
method.
>
> > return true;
> > }
> > return false;
> > @@ -438,9 +458,24 @@
> > if (StateStack.back() == inMapFirstKey) {
> > StateStack.pop_back();
> > StateStack.push_back(inMapOtherKey);
> > + } else if (StateStack.back() == inFlowMapFirstKey) {
> > + StateStack.pop_back();
> > + StateStack.push_back(inFlowMapOtherKey);
> > }
> > }
> >
> > +void Output::beginFlowMapping() {
> > + StateStack.push_back(inFlowMapFirstKey);
> > + this->newLineCheck();
> > + ColumnAtMapFlowStart = Column;
> > + output("{ ");
> > +}
> > +
> > +void Output::endFlowMapping() {
> > + StateStack.pop_back();
> > + this->outputUpToEndOfLine(" }");
> > +}
> > +
> > void Output::beginDocuments() {
> > this->outputUpToEndOfLine("---");
> > }
> > @@ -607,7 +642,9 @@
> >
> > void Output::outputUpToEndOfLine(StringRef s) {
> > this->output(s);
> > - if (StateStack.empty() || StateStack.back() != inFlowSeq)
> > + if (StateStack.empty() || (StateStack.back() != inFlowSeq &&
> > + StateStack.back() != inFlowMapFirstKey &&
> > + StateStack.back() != inFlowMapOtherKey))
> > NeedsNewLine = true;
> > }
> >
> > @@ -634,7 +671,8 @@
> > if (StateStack.back() == inSeq) {
> > OutputDash = true;
> > } else if ((StateStack.size() > 1) && ((StateStack.back() ==
> inMapFirstKey) ||
> > - (StateStack.back() == inFlowSeq)) &&
> > + (StateStack.back() == inFlowSeq) ||
> > + (StateStack.back() == inFlowMapFirstKey)) &&
> > (StateStack[StateStack.size() - 2] == inSeq)) {
> > --Indent;
> > OutputDash = true;
> > Index: unittests/Support/YAMLIOTest.cpp
> > ===================================================================
> > --- unittests/Support/YAMLIOTest.cpp
> > +++ unittests/Support/YAMLIOTest.cpp
> > @@ -1367,6 +1367,91 @@
> > EXPECT_TRUE(!!yin.error());
> > }
> >
> >
> +//===----------------------------------------------------------------------===//
> > +// Test flow mapping
> >
> +//===----------------------------------------------------------------------===//
> > +
> > +struct FlowFooBar {
> > + int foo;
> > + int bar;
> > +
> > + FlowFooBar() : foo(0), bar(0) {}
> > + FlowFooBar(int foo, int bar) : foo(foo), bar(bar) {}
> > +};
> > +
> > +typedef std::vector<FlowFooBar> FlowFooBarSequence;
> > +
> > +LLVM_YAML_IS_SEQUENCE_VECTOR(FlowFooBar)
> > +
> > +struct FlowFooBarDoc {
> > + FlowFooBar attribute;
> > + FlowFooBarSequence seq;
> > +};
> > +
> > +namespace llvm {
> > +namespace yaml {
> > + template <>
> > + struct MappingTraits<FlowFooBar> {
> > + static void mapping(IO &io, FlowFooBar &fb) {
> > + io.mapRequired("foo", fb.foo);
> > + io.mapRequired("bar", fb.bar);
> > + }
> > +
> > + static const bool flow = true;
> > + };
> > +
> > + template <>
> > + struct MappingTraits<FlowFooBarDoc> {
> > + static void mapping(IO &io, FlowFooBarDoc &fb) {
> > + io.mapRequired("attribute", fb.attribute);
> > + io.mapRequired("seq", fb.seq);
> > + }
> > + };
> > +}
> > +}
> > +
> > +//
> > +// Test writing then reading back custom mappings
> > +//
> > +TEST(YAMLIO, TestReadWriteMyFlowMapping) {
> > + std::string intermediate;
> > + {
> > + FlowFooBarDoc doc;
> > + doc.attribute = FlowFooBar(42, 907);
> > + doc.seq.push_back(FlowFooBar(1, 2));
> > + doc.seq.push_back(FlowFooBar(0, 0));
> > + doc.seq.push_back(FlowFooBar(-1, 1024));
> > +
> > + llvm::raw_string_ostream ostr(intermediate);
> > + Output yout(ostr);
> > + yout << doc;
> > +
> > + // Verify that mappings were written in flow style
> > + ostr.flush();
> > + llvm::StringRef flowOut(intermediate);
> > + EXPECT_NE(llvm::StringRef::npos, flowOut.find("{ foo: 42, bar: 907
> }"));
> > + EXPECT_NE(llvm::StringRef::npos, flowOut.find("- { foo: 1, bar: 2
> }"));
> > + EXPECT_NE(llvm::StringRef::npos, flowOut.find("- { foo: 0, bar: 0
> }"));
> > + EXPECT_NE(llvm::StringRef::npos, flowOut.find("- { foo: -1, bar:
> 1024 }"));
> > + }
> > +
> > + {
> > + Input yin(intermediate);
> > + FlowFooBarDoc doc2;
> > + yin >> doc2;
> > +
> > + EXPECT_FALSE(yin.error());
> > + EXPECT_EQ(doc2.attribute.foo, 42);
> > + EXPECT_EQ(doc2.attribute.bar, 907);
> > + EXPECT_EQ(doc2.seq.size(), 3UL);
> > + EXPECT_EQ(doc2.seq[0].foo, 1);
> > + EXPECT_EQ(doc2.seq[0].bar, 2);
> > + EXPECT_EQ(doc2.seq[1].foo, 0);
> > + EXPECT_EQ(doc2.seq[1].bar, 0);
> > + EXPECT_EQ(doc2.seq[2].foo, -1);
> > + EXPECT_EQ(doc2.seq[2].bar, 1024);
> > + }
> > +}
> >
> >
> //===----------------------------------------------------------------------===//
> > // Test error handling
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150501/4952a7c2/attachment.html>
More information about the llvm-commits
mailing list