<div dir="ltr"><br><div class="gmail_extra"><br><div class="gmail_quote">On Tue, Oct 18, 2016 at 11:38 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:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class=""><br>
> On Oct 18, 2016, at 11:34 AM, Rong Xu <<a href="mailto:xur@google.com">xur@google.com</a>> wrote:<br>
><br>
><br>
><br>
> On Tue, Oct 18, 2016 at 11:14 AM, Vedant Kumar <<a href="mailto:vsk@apple.com">vsk@apple.com</a>> wrote:<br>
> 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>
> Sorry about this. Will add -U99999 later.<br>
><br>
><br>
><br>
> ================<br>
> Comment at: include/llvm/ProfileData/<wbr>InstrProfReader.h:108<br>
> +  /// 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>
> ----------------<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>
><br>
> I don't see why this breaks CoverageMapping::load() as this API is not referenced there.<br>
> It's only called in llvm-profiledata.cpp.<br>
<br>
</span>Sorry, my mistake.<br>
<span class=""><br>
<br>
><br>
> The difference InstrProfReader::create and IndexedInstrProfReader::create are intentional as the later will be referenced in CoverageMapping, Clang PGO and IR PGO etc.<br>
><br>
><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>
><br>
> yes. this also works.<br>
<br>
</span>I still prefer this if only for the API consistency.<br></blockquote><div><br></div><div>Sure. I'll update the patch with this approach.</div><div><br></div><div>thanks!</div><div> <br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
thanks,<br>
vedant<br>
<div class="HOEnZb"><div class="h5"><br>
><br>
><br>
><br>
> ================<br>
> Comment at: tools/llvm-profdata/llvm-<wbr>profdata.cpp:149<br>
> +  if (!Reader)<br>
> +    return;<br>
>    bool IsIRProfile = Reader->isIRLevelProfile();<br>
> ----------------<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>
><br>
<br>
</div></div></blockquote></div><br></div></div>