[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

774

Looks like you could move this to before the if?

[grimar]

lib/Support/YAMLTraits.cpp

779

This seems can be a bit simpler:

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

Column = 0;

779

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

779

Looks like you could move this to before the if?

I can't..

[grimar]

lib/Support/YAMLTraits.cpp

779

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

779

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;
+  StringRef PaddingBeforeContainer;
 };
 
 /// 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;
+  PaddingBeforeContainer = Padding;
+  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)
+  if (StateStack.back() == inMapFirstKey) {
+    Padding = PaddingBeforeContainer;
+    newLineCheck();
     output("{}");
+    Padding = "\n";
+  }
   StateStack.pop_back();
 }
 
 std::vector<StringRef> Output::keys() {
   report_fatal_error("invalid call");
 }
 
 bool Output::preflightKey(const char *Key, bool Required, bool SameAsDefault,

 }
 
 void Output::endDocuments() {
   output("\n...\n");
 }
 
 unsigned Output::beginSequence() {
   StateStack.push_back(inSeqFirstElement);
-  NeedsNewLine = true;
+  PaddingBeforeContainer = Padding;
+  Padding = "\n";
   return 0;
 }
 
 void Output::endSequence() {
   // If we did not emit anything, we should explicitly emit an empty sequence
-  if (StateStack.back() == inSeqFirstElement)
+  if (StateStack.back() == inSeqFirstElement) {
+    Padding = PaddingBeforeContainer;
+    newLineCheck();
     output("[]");
+    Padding = "\n";
+  }
   StateStack.pop_back();
 }
 
 bool Output::preflightElement(unsigned, void *&) {
   return true;
 }
 
 void Output::postflightElement(void *) {
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)
+  if (Padding != "\n") {
+    output(Padding);
+    Padding = {};
     return;
-  NeedsNewLine = false;
-
+  }
   outputNewLine();
+  Padding = {};
 
   if (StateStack.size() == 0)
     return;
 
   unsigned Indent = StateStack.size() - 1;
   bool OutputDash = false;
 
   if (StateStack.back() == inSeqFirstElement ||
       StateStack.back() == inSeqOtherElement) {

 
 }
 
 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/BinaryFormat/MsgPackDocumentTest.cpp

TEST(MsgPackDocument, TestOutputYAMLMap) {
   M["qux"] = N;
   N["baz"] = Doc.getNode(true);
   std::string Buffer;
   raw_string_ostream OStream(Buffer);
   Doc.toYAML(OStream);
   ASSERT_EQ(OStream.str(), "---\n"
                            "bar:             2\n"
                            "foo:             1\n"
-                           "qux:             \n"
+                           "qux:\n"
                            "  baz:             true\n"
                            "...\n");
 }
 
 TEST(MsgPackDocument, TestOutputYAMLMapHex) {
   Document Doc;
   Doc.setHexMode();
   auto M = Doc.getRoot().getMap(/*Convert=*/true);
   M["foo"] = Doc.getNode(int64_t(1));
   M["bar"] = Doc.getNode(uint64_t(2));
   auto N = Doc.getMapNode();
   M["qux"] = N;
   N["baz"] = Doc.getNode(true);
   std::string Buffer;
   raw_string_ostream OStream(Buffer);
   Doc.toYAML(OStream);
   ASSERT_EQ(OStream.str(), "---\n"
                            "bar:             0x2\n"
                            "foo:             1\n"
-                           "qux:             \n"
+                           "qux:\n"
                            "  baz:             true\n"
                            "...\n");
 }
 
 TEST(MsgPackDocument, TestInputYAMLMap) {
   Document Doc;
   bool Ok = Doc.fromYAML("---\nfoo: !int 0x1\nbaz: !str 2\n...\n");
   ASSERT_TRUE(Ok);

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();
 }
 
 LLVM_YAML_IS_STRING_MAP(int)
 
 TEST(YAMLIO, TestCustomMapping) {
   std::map<std::string, int> x;
-  x["foo"] = 1;
-  x["bar"] = 2;
 
   std::string out;
   llvm::raw_string_ostream ostr(out);
   Output xout(ostr, nullptr, 0);
 
   xout << x;
   ostr.flush();
   EXPECT_EQ("---\n"
+            "{}\n"
+            "...\n",
+            out);
+
+  x["foo"] = 1;
+  x["bar"] = 2;
+
+  out.clear();
+  xout << x;
+  ostr.flush();
+  EXPECT_EQ("---\n"
             "bar:             2\n"
             "foo:             1\n"
             "...\n",
             out);
 
   Input yin(out);
   std::map<std::string, int> y;
   yin >> y;
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;
   EXPECT_EQ(2ul, y.size());
   EXPECT_EQ(1, y["foo"].foo);
   EXPECT_EQ(2, y["foo"].bar);
   EXPECT_EQ(3, y["bar"].foo);
   EXPECT_EQ(4, y["bar"].bar);
 }
 
+struct FooBarMapMap {
+  std::map<std::string, FooBar> fbm;
+};
+
+template <> struct MappingTraits<FooBarMapMap> {
+  static void mapping(IO &io, FooBarMapMap &x) {
+    io.mapRequired("fbm", x.fbm);
+  }
+};
+
+TEST(YAMLIO, TestEmptyMapWrite) {
+  FooBarMapMap cont;
+  std::string str;
+  llvm::raw_string_ostream OS(str);
+  Output yout(OS);
+  yout << cont;
+  EXPECT_EQ(OS.str(), "---\nfbm:             {}\n...\n");
+}
+
+TEST(YAMLIO, TestEmptySequenceWrite) {
+  FooBarContainer cont;
+  std::string str;
+  llvm::raw_string_ostream OS(str);
+  Output yout(OS);
+  yout << cont;
+  EXPECT_EQ(OS.str(), "---\nfbs:             []\n...\n");
+}
+
 static void TestEscaped(llvm::StringRef Input, llvm::StringRef Expected) {
   std::string out;
   llvm::raw_string_ostream ostr(out);
   Output xout(ostr, nullptr, 0);
 
   llvm::yaml::EmptyContext Ctx;
   yamlize(xout, Input, true, Ctx);
 

unittests/TextAPI/ELFYAMLTest.cpp

TEST(ElfYamlTextAPI, YAMLUnreadableTBE) {
   ASSERT_THAT_ERROR(StubOrErr.takeError(), Failed());
 }
 
 TEST(ElfYamlTextAPI, YAMLWritesTBESymbols) {
   const char Expected[] =
       "--- !tapi-tbe\n"
       "TbeVersion:      1.0\n"
       "Arch:            AArch64\n"
-      "Symbols:         \n"
+      "Symbols:\n"
       "  bar:             { Type: Func, Weak: true }\n"
       "  foo:             { Type: NoType, Size: 99, Warning: Does nothing }\n"
       "  nor:             { Type: Func, Undefined: true }\n"
       "  not:             { Type: Unknown, Size: 12345678901234 }\n"
       "...\n";
   ELFStub Stub;
   Stub.TbeVersion = VersionTuple(1, 0);
   Stub.Arch = ELF::EM_AARCH64;
TEST(ElfYamlTextAPI, YAMLWritesTBESymbols) {
   compareByLine(Result.c_str(), Expected);
 }
 
 TEST(ElfYamlTextAPI, YAMLWritesNoTBESyms) {
   const char Expected[] = "--- !tapi-tbe\n"
                           "TbeVersion:      1.0\n"
                           "SoName:          nosyms.so\n"
                           "Arch:            x86_64\n"
-                          "NeededLibs:      \n"
+                          "NeededLibs:\n"
                           "  - libc.so\n"
                           "  - libfoo.so\n"
                           "  - libbar.so\n"
                           "Symbols:         {}\n"
                           "...\n";
   ELFStub Stub;
   Stub.TbeVersion = VersionTuple(1, 0);
   Stub.SoName = "nosyms.so";

unittests/TextAPI/TextStubV1Tests.cpp

static const char tbd_v1_file3[] =
       "---\n"
       "archs:           [ i386, x86_64 ]\n"
       "platform:        macosx\n"
       "install-name:    '/usr/lib/libfoo.dylib'\n"
       "current-version: 1.2.3\n"
       "compatibility-version: 0\n"
       "swift-version:   5\n"
       "objc-constraint: retain_release\n"
-      "exports:         \n"
+      "exports:\n"
       "  - archs:           [ i386 ]\n"
       "    symbols:         [ _sym1 ]\n"
       "    weak-def-symbols: [ _sym2 ]\n"
       "    thread-local-symbols: [ _sym3 ]\n"
       "  - archs:           [ x86_64 ]\n"
       "    allowed-clients: [ clientA ]\n"
       "    re-exports:      [ '/usr/lib/libfoo.dylib' ]\n"
       "    symbols:         [ '_OBJC_EHTYPE_$_Class1' ]\n"

unittests/TextAPI/TextStubV2Tests.cpp

TEST(TBDv2, WriteFile) {
   static const char tbd_v2_file3[] =
       "--- !tapi-tbd-v2\n"
       "archs:           [ i386, x86_64 ]\n"
       "platform:        macosx\n"
       "install-name:    '/usr/lib/libfoo.dylib'\n"
       "current-version: 1.2.3\n"
       "compatibility-version: 0\n"
       "swift-version:   5\n"
-      "exports:         \n"
+      "exports:\n"
       "  - archs:           [ i386 ]\n"
       "    symbols:         [ _sym1 ]\n"
       "    weak-def-symbols: [ _sym2 ]\n"
       "    thread-local-symbols: [ _sym3 ]\n"
       "  - archs:           [ x86_64 ]\n"
       "    allowable-clients: [ clientA ]\n"
       "    re-exports:      [ '/usr/lib/libfoo.dylib' ]\n"
       "    symbols:         [ '_OBJC_EHTYPE_$_Class1' ]\n"