<div dir="ltr">Thanks Jonathan!<div><br></div><div>I have a couple of inline comments below:</div><div><br></div><div><div>> diff --git a/include/llvm/Support/YAMLTraits.h b/include/llvm/Support/YAMLTraits.h</div><div>> index c04294a..6ba839c 100644</div><div>> --- a/include/llvm/Support/YAMLTraits.h</div><div>> +++ b/include/llvm/Support/YAMLTraits.h</div><div>> @@ -976,6 +976,10 @@ public:</div><div>>          void *Ctxt = nullptr,</div><div>>          SourceMgr::DiagHandlerTy DiagHandler = nullptr,</div><div>>          void *DiagHandlerCtxt = nullptr);</div><div>> +  Input(MemoryBuffer &Input,</div><div><br></div><div>I think we should pass in a MemoryBufferRef here.</div><div>This would make the API more consistent with the YAML stream's API.</div><div><br></div><div>> +        void *Ctxt = nullptr,</div><div>> +        SourceMgr::DiagHandlerTy DiagHandler = nullptr,</div><div>> +        void *DiagHandlerCtxt = nullptr);</div><div>>    ~Input() override;</div><div>>  </div><div>>    // Check if there was an syntax or semantic error during parsing.</div><div>> diff --git a/lib/Support/YAMLTraits.cpp b/lib/Support/YAMLTraits.cpp</div><div>> index 6b59a16..9f1cb87 100644</div><div>> --- a/lib/Support/YAMLTraits.cpp</div><div>> +++ b/lib/Support/YAMLTraits.cpp</div><div>> @@ -56,6 +56,19 @@ Input::Input(StringRef InputContent,</div><div>>    DocIterator = Strm->begin();</div><div>> }</div><div>>  </div><div>> +Input::Input(MemoryBuffer &Input,</div><div>> +             void *Ctxt,</div><div>> +             SourceMgr::DiagHandlerTy DiagHandler,</div><div>> +             void *DiagHandlerCtxt)</div><div>> +  : IO(Ctxt),</div><div>> +    Strm(new Stream(</div><div>> +      MemoryBufferRef(Input.getBuffer(), Input.getBufferIdentifier()), SrcMgr)),</div><div>> +    CurrentNode(nullptr) {</div><div>> +  if (DiagHandler)</div><div>> +    SrcMgr.setDiagHandler(DiagHandler, DiagHandlerCtxt);</div><div>> +  DocIterator = Strm->begin();</div><div>> +}</div><div>> +</div><div>>  Input::~Input() {</div><div>>  }</div><div>>  </div><div>> diff --git a/unittests/Support/YAMLIOTest.cpp b/unittests/Support/YAMLIOTest.cpp</div><div>> index e7affa1..9b34109 100644</div><div>> --- a/unittests/Support/YAMLIOTest.cpp</div><div>> +++ b/unittests/Support/YAMLIOTest.cpp</div><div>> @@ -233,6 +233,15 @@ TEST(YAMLIO, TestSequenceMapWriteAndRead) {</div><div>>    }</div><div>>  }</div><div>>  </div><div>> +//</div><div>> +// Test YAML filename handling.</div><div>> +//</div><div>> +TEST(YAMLIO, TestGivenFilename) {</div><div>> +  // NOTE: Input doesn't expose enough information to retrieve the filename.</div><div>> +  auto Buffer = llvm::MemoryBuffer::getMemBuffer("{ x: 42 }", "foo.yaml");</div><div>> +  Input yin(*Buffer);</div><div>> +}</div><div><br></div><div>While we can't access the filename directly, we still would like to test that </div><div>now the yaml::Input class uses the filename from the provided buffer </div><div>reference.</div><div>We can do it by checking that the reported diagnostics actually use the</div><div>specified filename, like in the example below:</div><div><br></div><div><div>  static void testErrorFilename(const llvm::SMDiagnostic &Error, void *) {</div><div>    EXPECT_EQ(Error.getFilename(), "foo.yaml");</div><div>  }</div><div><br></div><div>  TEST(YAMLIO, TestGivenFilename) {</div><div>    auto Buffer = llvm::MemoryBuffer::getMemBuffer("{ x: 42 }", "foo.yaml");</div><div>    Input yin(Buffer->getMemBufferRef(), nullptr, testErrorFilename);</div><div>    FooBar Value;</div><div>    yin >> Value;</div><div>  </div><div>    EXPECT_TRUE(!!yin.error());</div><div>  }</div></div><div><br></div><div>Please update the test case to perform some kind of check like this. </div><div>Feel free to <span style="line-height:normal">use the example code I posted above.</span></div><div><br></div><div><br></div><div>> +</div></div></div><div class="gmail_extra"><br><div class="gmail_quote">2015-07-24 9:21 GMT-07:00 Jonathan Anderson <span dir="ltr"><<a href="mailto:jonathan.anderson@mun.ca" target="_blank">jonathan.anderson@mun.ca</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hello llvm-commits,<br>
<br>
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".<br>
<br>
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.<br>
<br>
There’s also a Phabricator review open at <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D11137&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=Z2ZE0clFS4GFxpAaO6N0JDWR3F6MXLuLRYfLfoaGMOc&s=YBeRy8EKmLeLwH3lmRvVxFJKG3kyUarppJXvwd74mQU&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11137</a>, but I’ve also attached the patch for those who prefer that workflow.<br>
<br>
Thanks very much to Alex Lorenz and Duncan Smith for their helpful comments on the patch and the process for getting it reviewed.<br>
<br>
<br>
Jonathan Anderson<br>
--<br>
Assistant Professor<br>
Electrical and Computer Engineering<br>
Memorial University of Newfoundland<br>
<br>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__www.engr.mun.ca_-7Eanderson&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=Z2ZE0clFS4GFxpAaO6N0JDWR3F6MXLuLRYfLfoaGMOc&s=y0hc0C0QGmHxSFX7gOlkOfIs5FDROuJc2YA9mtnHLGA&e=" rel="noreferrer" target="_blank">http://www.engr.mun.ca/~anderson</a></blockquote></div><br></div>