[PATCH] D70759: [llvm-symbolizer] Support debug file lookup using build ID

[Lang Hames via Phabricator via llvm-commits]

lhames added inline comments.


================
Comment at: llvm/lib/DebugInfo/Symbolize/Symbolize.cpp:303
+        return N.getDesc();
+  }
+  return {};
----------------
dblaikie wrote:
> dblaikie wrote:
> > vsk wrote:
> > > Does `Err` need to be checked here, after the loop?
> > > 
> > > From `notes_begin`:
> > > 
> > >   /// \param Err [out] an error to support fallible iteration, which should
> > >   ///  be checked after iteration ends.
> > > 
> > Looks like it to me.
> > 
> > A test case would be good/necessary, though - @lhames - you wouldn't happen to know a good way to produce an object file with this problem, by chance?
> Also is there anything to be done about this idiom to make it more robust like other uses of llvm::Error? So that this usage would fail even in Success-only executions.
I think it should already fail if you don't check Err, even on success. The caveat is that you would have to exit the bottom of the loop: If you always leave via that return statement (which you might have if that's what you were testing) then you aren't forced to check the error, because returns from inside the loop body are guaranteed to be safe. Making safe returns not require checking was actually one of the main reasons we developed the fallible iterator wrapper in the first place, if memory serves. :) 


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70759/new/

https://reviews.llvm.org/D70759