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