[PATCH] [YAMLIO] Make line-wrapping configurable and test it.

[Justin Bogner]

Frederic Riss <friss at apple.com> writes:
> Hi bogner,
>
> We would wrap flow mappings and sequences when they go over a hardcoded 70
> characters limit. Make the wrapping column configurable (and default to 70
> co the change should be NFC for current users). Passing 0 allows to completely
> suppress the wrapping which makes it easier to handle in tools like FileCheck.

LGTM, with a minor comment.

> http://reviews.llvm.org/D10109
>
> Files:
>   docs/YamlIO.rst
>   include/llvm/Support/YAMLTraits.h
>   lib/Support/YAMLTraits.cpp
>   unittests/Support/YAMLIOTest.cpp
>
> EMAIL PREFERENCES
>   http://reviews.llvm.org/settings/panel/emailpreferences/
>
> Index: docs/YamlIO.rst
> ===================================================================
> --- docs/YamlIO.rst
> +++ docs/YamlIO.rst
> @@ -798,6 +798,7 @@
>        static const bool flow = true;
>      }
>  
> +Flow mappings are subject to line wrapping according to the Output object configuration.
>  
>  Sequence
>  ========
> @@ -845,6 +846,7 @@
>  structures, then when converted to YAML, a flow sequence of integers 
>  will be used (e.g. [ 10, -3, 4 ]).
>  
> +Flow sequences are subject to line wrapping according to the Output object configuration.

Could you subject these lines to line wrapping? ;)

>  
>  Utility Macros
>  --------------
> @@ -908,22 +910,26 @@
>  
>  The llvm::yaml::Output class is used to generate a YAML document from your 
>  in-memory data structures, using traits defined on your data types.  
> -To instantiate an Output object you need an llvm::raw_ostream, and optionally 
> -a context pointer:
> +To instantiate an Output object you need an llvm::raw_ostream, an optional 
> +context pointer and an optional wrapping column:
>  
>  .. code-block:: c++
>  
>        class Output : public IO {
>        public:
> -        Output(llvm::raw_ostream &, void *context=NULL);
> +        Output(llvm::raw_ostream &, void *context = NULL, int WrapColumn = 70);
>      
>  Once you have an Output object, you can use the C++ stream operator on it
>  to write your native data as YAML. One thing to recall is that a YAML file
>  can contain multiple "documents".  If the top level data structure you are
>  streaming as YAML is a mapping, scalar, or sequence, then Output assumes you
>  are generating one document and wraps the mapping output 
>  with  "``---``" and trailing "``...``".  
>  
> +The WrapColumn parameter will cause the flow mappings and sequences to
> +line-wrap when they go over the supplied column. Pass 0 to completely
> +suppress the wrapping.
> +
>  .. code-block:: c++
>     
>      using llvm::yaml::Output;
> Index: include/llvm/Support/YAMLTraits.h
> ===================================================================
> --- include/llvm/Support/YAMLTraits.h
> +++ include/llvm/Support/YAMLTraits.h
> @@ -1114,7 +1114,7 @@
>  ///
>  class Output : public IO {
>  public:
> -  Output(llvm::raw_ostream &, void *Ctxt=nullptr);
> +  Output(llvm::raw_ostream &, void *Ctxt = nullptr, int WrapColumn = 70);
>    ~Output() override;
>  
>    bool outputting() override;
> @@ -1170,6 +1170,7 @@
>    };
>  
>    llvm::raw_ostream       &Out;
> +  int                      WrapColumn;
>    SmallVector<InState, 8>  StateStack;
>    int                      Column;
>    int                      ColumnAtFlowStart;
> Index: lib/Support/YAMLTraits.cpp
> ===================================================================
> --- lib/Support/YAMLTraits.cpp
> +++ lib/Support/YAMLTraits.cpp
> @@ -404,9 +404,10 @@
>  //  Output
>  //===----------------------------------------------------------------------===//
>  
> -Output::Output(raw_ostream &yout, void *context)
> +Output::Output(raw_ostream &yout, void *context, int WrapColumn)
>      : IO(context),
>        Out(yout),
> +      WrapColumn(WrapColumn),
>        Column(0),
>        ColumnAtFlowStart(0),
>        ColumnAtMapFlowStart(0),
> @@ -529,7 +530,7 @@
>  bool Output::preflightFlowElement(unsigned, void *&) {
>    if (NeedFlowSequenceComma)
>      output(", ");
> -  if (Column > 70) {
> +  if (WrapColumn && Column > WrapColumn) {
>      output("\n");
>      for (int i = 0; i < ColumnAtFlowStart; ++i)
>        output(" ");
> @@ -720,7 +721,7 @@
>  void Output::flowKey(StringRef Key) {
>    if (StateStack.back() == inFlowMapOtherKey)
>      output(", ");
> -  if (Column > 70) {
> +  if (WrapColumn && Column > WrapColumn) {
>      output("\n");
>      for (int I = 0; I < ColumnAtMapFlowStart; ++I)
>        output(" ");
> Index: unittests/Support/YAMLIOTest.cpp
> ===================================================================
> --- unittests/Support/YAMLIOTest.cpp
> +++ unittests/Support/YAMLIOTest.cpp
> @@ -2074,3 +2074,123 @@
>    EXPECT_FALSE(yin.error());
>    EXPECT_TRUE(seq.empty());
>  }
> +
> +struct FlowMap {
> +  llvm::StringRef str1, str2, str3;
> +  FlowMap(llvm::StringRef str1, llvm::StringRef str2, llvm::StringRef str3)
> +    : str1(str1), str2(str2), str3(str3) {}
> +};
> +
> +namespace llvm {
> +namespace yaml {
> +  template <>
> +  struct MappingTraits<FlowMap> {
> +    static void mapping(IO &io, FlowMap &fm) {
> +      io.mapRequired("str1", fm.str1);
> +      io.mapRequired("str2", fm.str2);
> +      io.mapRequired("str3", fm.str3);
> +    }
> +
> +    static const bool flow = true;
> +  };
> +}
> +}
> +
> +struct FlowSeq {
> +  llvm::StringRef str;
> +  FlowSeq(llvm::StringRef S) : str(S) {}
> +  FlowSeq() = default;
> +};
> +
> +template <>
> +struct ScalarTraits<FlowSeq> {
> +  static void output(const FlowSeq &value, void*, llvm::raw_ostream &out) {
> +    out << value.str;
> +  }
> +  static StringRef input(StringRef scalar, void*, FlowSeq &value) {
> +    value.str = scalar;
> +    return "";
> +  }
> +
> +  static bool mustQuote(StringRef S) { return false; }
> +};
> +
> +LLVM_YAML_IS_FLOW_SEQUENCE_VECTOR(FlowSeq)
> +
> +TEST(YAMLIO, TestWrapFlow) {
> +  std::string out;
> +  llvm::raw_string_ostream ostr(out);
> +  FlowMap Map("This is str1", "This is str2", "This is str3");
> +  std::vector<FlowSeq> Seq;
> +  Seq.emplace_back("This is str1");
> +  Seq.emplace_back("This is str2");
> +  Seq.emplace_back("This is str3");
> +
> +  {
> +    // 20 is just bellow the total length of the first mapping field.
> +    // We should wreap at every element.
> +    Output yout(ostr, nullptr, 15);
> +
> +    yout << Map;
> +    ostr.flush();
> +    EXPECT_EQ(out,
> +              "---\n"
> +              "{ str1: This is str1, \n"
> +              "  str2: This is str2, \n"
> +              "  str3: This is str3 }\n"
> +              "...\n");
> +    out.clear();
> +
> +    yout << Seq;
> +    ostr.flush();
> +    EXPECT_EQ(out,
> +              "---\n"
> +              "[ This is str1, \n"
> +              "  This is str2, \n"
> +              "  This is str3 ]\n"
> +              "...\n");
> +    out.clear();
> +  }
> +  {
> +    // 25 will allow the second field to be output on the first line.
> +    Output yout(ostr, nullptr, 25);
> +
> +    yout << Map;
> +    ostr.flush();
> +    EXPECT_EQ(out,
> +              "---\n"
> +              "{ str1: This is str1, str2: This is str2, \n"
> +              "  str3: This is str3 }\n"
> +              "...\n");
> +    out.clear();
> +
> +    yout << Seq;
> +    ostr.flush();
> +    EXPECT_EQ(out,
> +              "---\n"
> +              "[ This is str1, This is str2, \n"
> +              "  This is str3 ]\n"
> +              "...\n");
> +    out.clear();
> +  }
> +  {
> +    // 0 means no wrapping.
> +    Output yout(ostr, nullptr, 0);
> +
> +    yout << Map;
> +    ostr.flush();
> +    EXPECT_EQ(out,
> +              "---\n"
> +              "{ str1: This is str1, str2: This is str2, str3: This is str3 }\n"
> +              "...\n");
> +    out.clear();
> +
> +    yout << Seq;
> +    ostr.flush();
> +    EXPECT_EQ(out,
> +              "---\n"
> +              "[ This is str1, This is str2, This is str3 ]\n"
> +              "...\n");
> +    out.clear();
> +  }
> +}