[PATCH] D121801: [llmv-pdbutil] Replace ExitOnError with explicit error handling.

[Carlos Alberto Enciso via Phabricator via llvm-commits]

CarlosAlbertoEnciso added inline comments.


================
Comment at: llvm/tools/llvm-pdbutil/DumpOutputStyle.cpp:455
 
-  ExitOnError Err("Unexpected error processing modules: ");
-
----------------
aganea wrote:
> CarlosAlbertoEnciso wrote:
> > aganea wrote:
> > > The user would loose the context after removal of this line, if the message is printed on stderr. Can you move this line to `L201`, or replace it by an extra `StringError` if you plan on moving this entire file to the PDB library? It could be something like:
> > > ```
> > >   if (opts::dump::DumpSymbols) {
> > >     auto EC = File.isPdb() ? dumpModuleSymsForPdb() : dumpModuleSymsForObj();
> > >     if (EC)
> > >       return joinErrors(createStringError(inconvertibleErrorCode(), "Unexpected error processing modules:"), EC);
> > >   }
> > > ```
> > Very valid point.
> > 
> > The following functions are affected:
> > 
> > ```
> > Error DumpOutputStyle::dumpModules();
> > Error DumpOutputStyle::dumpModuleFiles();
> > Error DumpOutputStyle::dumpSymbolStats();
> > Error DumpOutputStyle::dumpModuleSymsForPdb();
> > 
> > ```
> > What I am suggesting is to pass an additional parameter to the functions that are intended to be moved to the PDB library that now are returning an Error and add your suggested code to those functions.
> > 
> > ```
> > if (EC)
> >     return joinErrors(createStringError(inconvertibleErrorCode(), ExtraMessage), EC);
> > ```
> > 
> I guess you can also move the `ExitOnError` closer to the caller site:
> ```
> Error DumpOutputStyle::dump() {
>   ...
>   if (opts::dump::DumpModules) {
>     ExitOnError Err("Unexpected error processing modules: ");
>     Err(dumpModules());
>   }
> ```
Added your suggested change to the caller sites that are missing the `ExitOnError`.


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

https://reviews.llvm.org/D121801