[YAMLIO] Remove trailing spaces when outputting maps

[MaskRay]

Summary

llvm::yaml::Output::paddedKey unconditionally outputs spaces, which
are superfluous if the value to be dumped is a sequence or map.
Change bool NeedsNewLine to StringRef Padding so that it can be
overridden to \n if the value is a sequence or map.

An empty map/sequence is special. It is printed as {} or [] without
a newline, while a non-empty map/sequence follows a newline. To handle
this distinction, add another variable PaddingBeforeContainer and does
the special handling in endMapping/endSequence.

[jhenderson]

lib/Support/YAMLTraits.cpp

764

Looks like you could move this to before the if?

[grimar]

lib/Support/YAMLTraits.cpp

768

This seems can be a bit simpler:

Out << Padding;
Padding = "";
if (Padding != "\n")
  return;

Column = 0;

768

Ah, no. My mistake.

[grimar]

Looks fine, but someone else should also confirm (I am not really familar with the code here).

[MaskRay]

lib/Support/YAMLTraits.cpp

768

Looks like you could move this to before the if?

I can't..

[grimar]

lib/Support/YAMLTraits.cpp

768

Interesting that James seems tried to suggest the same. Looks there is something in this code pattern that make people think it can be simplified in this way..

[jhenderson]

LGTM, I think too.

lib/Support/YAMLTraits.cpp

768

Yup. My bad too!

[MaskRay]

Fix output of empty sequence/map

[MaskRay]

Update tests

include/llvm/Support/YAMLTraits.h

private:
   int WrapColumn;
   SmallVector<InState, 8> StateStack;
   int Column = 0;
   int ColumnAtFlowStart = 0;
   int ColumnAtMapFlowStart = 0;
   bool NeedBitValueComma = false;
   bool NeedFlowSequenceComma = false;
   bool EnumerationMatchFound = false;
-  bool NeedsNewLine = false;
   bool WriteDefaultValues = false;
+  StringRef Padding;
 };
 
 /// YAML I/O does conversion based on types. But often native data types
 /// are just a typedef of built in intergral types (e.g. int).  But the C++
 /// type matching system sees through the typedef and all the typedefed types
 /// look like a built in type. This will cause the generic YAML I/O conversion
 /// to be used. To provide better control over the YAML conversion, you can
 /// use this macro instead of typedef.  It will create a class with one field

lib/Support/YAMLTraits.cpp

 Output::~Output() = default;
 
 bool Output::outputting() {
   return true;
 }
 
 void Output::beginMapping() {
   StateStack.push_back(inMapFirstKey);
-  NeedsNewLine = true;
+  Padding = "\n";
 }
 
 bool Output::mapTag(StringRef Tag, bool Use) {
   if (Use) {
     // If this tag is being written inside a sequence we should write the start
     // of the sequence before writing the tag, otherwise the tag won't be
     // attached to the element in the sequence, but rather the sequence itself.
     bool SequenceElement = false;
if (SequenceElement) {
       // If we're writing the tag during the first element of a map, the tag
       // takes the place of the first element in the sequence.
       if (StateStack.back() == inMapFirstKey) {
         StateStack.pop_back();
         StateStack.push_back(inMapOtherKey);
       }
       // Tags inside maps in sequences should act as keys in the map from a
       // formatting perspective, so we always want a newline in a sequence.
-      NeedsNewLine = true;
+      Padding = "\n";
     }
   }
   return Use;
 }
 
 void Output::endMapping() {
   // If we did not map anything, we should explicitly emit an empty map
   if (StateStack.back() == inMapFirstKey)

 }
 
 void Output::endDocuments() {
   output("\n...\n");
 }
 
 unsigned Output::beginSequence() {
   StateStack.push_back(inSeqFirstElement);
-  NeedsNewLine = true;
+  Padding = "\n";
   return 0;
 }
 
 void Output::endSequence() {
   // If we did not emit anything, we should explicitly emit an empty sequence
   if (StateStack.back() == inSeqFirstElement)
     output("[]");
   StateStack.pop_back();
void Output::output(StringRef s) {
   Column += s.size();
   Out << s;
 }
 
 void Output::outputUpToEndOfLine(StringRef s) {
   output(s);
   if (StateStack.empty() || (!inFlowSeqAnyElement(StateStack.back()) &&
                              !inFlowMapAnyKey(StateStack.back())))
-    NeedsNewLine = true;
+    Padding = "\n";
 }
 
 void Output::outputNewLine() {
   Out << "\n";
   Column = 0;
 }
 
 // if seq at top, indent as if map, then add "- "
 // if seq in middle, use "- " if firstKey, else use "  "
 //
 
 void Output::newLineCheck() {
-  if (!NeedsNewLine)
+  Out << Padding;
+  if (Padding != "\n") {
+    Padding = {};
     return;
-  NeedsNewLine = false;
-
-  outputNewLine();
+  }
+  Padding = {};
+  Column = 0;
 
   if (StateStack.size() == 0)
     return;
 
   unsigned Indent = StateStack.size() - 1;
   bool OutputDash = false;
 
   if (StateStack.back() == inSeqFirstElement ||

 
 }
 
 void Output::paddedKey(StringRef key) {
   output(key);
   output(":");
   const char *spaces = "                ";
   if (key.size() < strlen(spaces))
-    output(&spaces[key.size()]);
+    Padding = &spaces[key.size()];
   else
-    output(" ");
+    Padding = " ";
 }
 
 void Output::flowKey(StringRef Key) {
   if (StateStack.back() == inFlowMapOtherKey)
     output(", ");
   if (WrapColumn && Column > WrapColumn) {
     output("\n");
     for (int I = 0; I < ColumnAtMapFlowStart; ++I)

unittests/Support/YAMLIOTest.cpp

TEST(YAMLIO, TestMapWithContext) {
   llvm::raw_string_ostream ostr(out);
 
   Output yout(ostr, nullptr, 15);
 
   yout << Nested;
   ostr.flush();
   EXPECT_EQ(1, Context.A);
   EXPECT_EQ("---\n"
-            "Simple:          \n"
+            "Simple:\n"
             "  B:               0\n"
             "  C:               0\n"
             "  Context:         1\n"
             "...\n",
             out);
 
   out.clear();
 
   Nested.Simple.B = 2;
   Nested.Simple.C = 3;
   yout << Nested;
   ostr.flush();
   EXPECT_EQ(2, Context.A);
   EXPECT_EQ("---\n"
-            "Simple:          \n"
+            "Simple:\n"
             "  B:               2\n"
             "  C:               3\n"
             "  Context:         2\n"
             "...\n",
             out);
   out.clear();
 }
 
TEST(YAMLIO, TestCustomMappingStruct) {
 
   std::string out;
   llvm::raw_string_ostream ostr(out);
   Output xout(ostr, nullptr, 0);
 
   xout << x;
   ostr.flush();
   EXPECT_EQ("---\n"
-            "bar:             \n"
+            "bar:\n"
             "  foo:             3\n"
             "  bar:             4\n"
-            "foo:             \n"
+            "foo:\n"
             "  foo:             1\n"
             "  bar:             2\n"
             "...\n",
             out);
 
   Input yin(out);
   std::map<std::string, FooBar> y;
   yin >> y;