This is an archive of the discontinued LLVM Phabricator instance.

Add filename support to yaml::Input.
AbandonedPublic

Authored by trombonehero on Jul 13 2015, 5:38 AM.

Details

Summary

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 commit 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.

Diff Detail

Event Timeline

trombonehero retitled this revision from to Add filename support to yaml::Input..
trombonehero updated this object.
trombonehero added a reviewer: theraven.
trombonehero edited reviewers, added: chandlerc; removed: theraven.Jul 13 2015, 5:45 AM

Hi Jonathan,

This patch lacks an appropriate test, please add a test case for the new constructor in the unit test file for yaml traits ('unittest/Support/YAMLIOTest.cpp').

A minor style comment is included below:

include/llvm/Support/YAMLTraits.h
979

The ampersand should be together with the name of the variable.
This should be done for the constructor in the CPP file as well.

  • Add unit test for new yaml::Input constructor.
trombonehero marked an inline comment as done.
  • Put reference ampersands with variable name.
trombonehero added subscribers: kledzik, bkramer.

Adding @bkramer and @kledzik as reviewers, since they seem to have written the majority of YAMLTraits.cpp.

  • Let yaml::Input take a MemoryBufferRef.
  • Make use of new MemoryBufferRef constructor from r253311.
emaste added a subscriber: emaste.Aug 19 2016, 8:41 AM
arphaman accepted this revision.Jan 30 2017, 10:28 AM

Please adjust the new constructor as requested, otherwise LGTM. Sorry that it took so long to get back.

lib/Support/YAMLTraits.cpp
63

Nitpick: on ToT the Input::Input above is formatted differently. It would be more appropriate to format the new constructor declaration similarly, something like should work:

Input::Input(MemoryBufferRef Input, void *Ctxt,
             SourceMgr::DiagHandlerTy DiagHandler, void *DiagHandlerCtxt)
    : IO(Ctxt), Strm(new Stream(Input, SrcMgr, false, &EC)),
      CurrentNode(nullptr) {
64

Please pass in &EC to the Stream constructor as the other Input constructor passes it into Stream on ToT.

This revision is now accepted and ready to land.Jan 30 2017, 10:28 AM
arphaman requested changes to this revision.Jan 31 2017, 10:06 AM

Jonathan, I have noticed that llvm-commits wasn't subscribed to this patch after I accepted it. I have to withdraw my LGTM, since patches to LLVM should go through llvm-commits according to the guidelines [1]. Can you please re-post this patch with llvm-commits added as a subscriber and me as a reviewer? Thank you for your understanding!

[1] http://llvm.org/docs/Phabricator.html

This revision now requires changes to proceed.Jan 31 2017, 10:06 AM
trombonehero abandoned this revision.Jul 13 2017, 4:51 PM