<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 18, 2016 at 11:14 AM, Vedant Kumar <span dir="ltr"><<a href="mailto:vsk@apple.com" target="_blank">vsk@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">vsk added a comment.<br>
<br>
Thanks for splitting the patch up. Please upload diffs with more context in the future (e.g git diff -U1000).<br>
<br></blockquote><div>Sorry about this. Will add -U99999 later.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: include/llvm/ProfileData/<wbr>InstrProfReader.h:108<br>
<span class="gmail-">+  /// instrprof file. Nullptr will be returned for an empty profile file.<br>
   static Expected<std::unique_ptr<<wbr>InstrProfReader>> create(const Twine &Path);<br>
<br>
</span>----------------<br>
This breaks CoverageMapping::load(), and creates an inconsistency between the API's for InstrProfReader::create and IndexedInstrProfReader::create (one can successfully return nullptr, the other can't).<br></blockquote><div> </div><div>I don't see why this breaks CoverageMapping::load() as this API is not referenced there. </div><div>It's only called in llvm-profiledata.cpp.</div><div><br></div><div>The difference InstrProfReader::create and IndexedInstrProfReader::create are intentional as the later will be referenced in CoverageMapping, Clang PGO and IR PGO etc.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
I'd prefer for this functionality to be implemented via a new instrprof_error. Clients can opt into handling this as a 'soft'/recoverable error, or fail with an appropriate diagnostic packaged up by the Error.<br></blockquote><div><br></div><div>yes. this also works.</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-color:rgb(204,204,204);border-left-style:solid;padding-left:1ex">
<br>
<br>
================<br>
Comment at: tools/llvm-profdata/llvm-<wbr>profdata.cpp:149<br>
<span class="gmail-">+  if (!Reader)<br>
+    return;<br>
   bool IsIRProfile = Reader->isIRLevelProfile();<br>
</span>----------------<br>
In llvm-profdata's loadInput, you could write:<br>
<br>
```<br>
if ((Error E = ReaderOrErr.takeError())) {<br>
  if (E != instrprof_error::empty_<wbr>profile)<br>
    WC->Err = std::move(E);<br>
  return;<br>
}<br>
```<br>
<br>
<br>
<a href="https://reviews.llvm.org/D25687" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D25687</a><br>
<br>
<br>
<br>
</blockquote></div><br></div></div>