[PATCH] YAML Input improvement

[Alex L]

Thanks Jonathan!

I have a couple of inline comments below:

> diff --git a/include/llvm/Support/YAMLTraits.h
b/include/llvm/Support/YAMLTraits.h
> index c04294a..6ba839c 100644
> --- a/include/llvm/Support/YAMLTraits.h
> +++ b/include/llvm/Support/YAMLTraits.h
> @@ -976,6 +976,10 @@ public:
>          void *Ctxt = nullptr,
>          SourceMgr::DiagHandlerTy DiagHandler = nullptr,
>          void *DiagHandlerCtxt = nullptr);
> +  Input(MemoryBuffer &Input,

I think we should pass in a MemoryBufferRef here.
This would make the API more consistent with the YAML stream's API.

> +        void *Ctxt = nullptr,
> +        SourceMgr::DiagHandlerTy DiagHandler = nullptr,
> +        void *DiagHandlerCtxt = nullptr);
>    ~Input() override;
>
>    // Check if there was an syntax or semantic error during parsing.
> diff --git a/lib/Support/YAMLTraits.cpp b/lib/Support/YAMLTraits.cpp
> index 6b59a16..9f1cb87 100644
> --- a/lib/Support/YAMLTraits.cpp
> +++ b/lib/Support/YAMLTraits.cpp
> @@ -56,6 +56,19 @@ Input::Input(StringRef InputContent,
>    DocIterator = Strm->begin();
> }
>
> +Input::Input(MemoryBuffer &Input,
> +             void *Ctxt,
> +             SourceMgr::DiagHandlerTy DiagHandler,
> +             void *DiagHandlerCtxt)
> +  : IO(Ctxt),
> +    Strm(new Stream(
> +      MemoryBufferRef(Input.getBuffer(), Input.getBufferIdentifier()),
SrcMgr)),
> +    CurrentNode(nullptr) {
> +  if (DiagHandler)
> +    SrcMgr.setDiagHandler(DiagHandler, DiagHandlerCtxt);
> +  DocIterator = Strm->begin();
> +}
> +
>  Input::~Input() {
>  }
>
> diff --git a/unittests/Support/YAMLIOTest.cpp
b/unittests/Support/YAMLIOTest.cpp
> index e7affa1..9b34109 100644
> --- a/unittests/Support/YAMLIOTest.cpp
> +++ b/unittests/Support/YAMLIOTest.cpp
> @@ -233,6 +233,15 @@ TEST(YAMLIO, TestSequenceMapWriteAndRead) {
>    }
>  }
>
> +//
> +// Test YAML filename handling.
> +//
> +TEST(YAMLIO, TestGivenFilename) {
> +  // NOTE: Input doesn't expose enough information to retrieve the
filename.
> +  auto Buffer = llvm::MemoryBuffer::getMemBuffer("{ x: 42 }",
"foo.yaml");
> +  Input yin(*Buffer);
> +}

While we can't access the filename directly, we still would like to test
that
now the yaml::Input class uses the filename from the provided buffer
reference.
We can do it by checking that the reported diagnostics actually use the
specified filename, like in the example below:

  static void testErrorFilename(const llvm::SMDiagnostic &Error, void *) {
    EXPECT_EQ(Error.getFilename(), "foo.yaml");
  }

  TEST(YAMLIO, TestGivenFilename) {
    auto Buffer = llvm::MemoryBuffer::getMemBuffer("{ x: 42 }", "foo.yaml");
    Input yin(Buffer->getMemBufferRef(), nullptr, testErrorFilename);
    FooBar Value;
    yin >> Value;

    EXPECT_TRUE(!!yin.error());
  }

Please update the test case to perform some kind of check like this.
Feel free to use the example code I posted above.


> +

2015-07-24 9:21 GMT-07:00 Jonathan Anderson <jonathan.anderson at mun.ca>:

> Hello llvm-commits,
>
> The current yaml::Input constructor only takes a StringRef for its first
> parameter: clients pass in raw string data. This means that, for the
> typical use case of opening a file and parsing it, we throw away the
> filename information and report errors as occurring in "YAML".
>
> This patch adds an alterate yaml::Input constructor that takes a reference
> to a MemoryBuffer, then extracts the buffer and whatever identifier is
> associated with that buffer and passes them to the underlying yaml::Stream
> as a MemoryBufferRef.
>
> There’s also a Phabricator review open at http://reviews.llvm.org/D11137,
> but I’ve also attached the patch for those who prefer that workflow.
>
> Thanks very much to Alex Lorenz and Duncan Smith for their helpful
> comments on the patch and the process for getting it reviewed.
>
>
> Jonathan Anderson
> --
> Assistant Professor
> Electrical and Computer Engineering
> Memorial University of Newfoundland
>
> http://www.engr.mun.ca/~anderson
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20150724/55d50e34/attachment.html>