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

[Reid Kleckner via Phabricator via llvm-commits]

rnk added a comment.

Essentially, this looks good to me. I have some minor concerns that as we migrate dumping functionality into library code, it will tend to increase the code size of production tools such as the linker. Maybe archive semantics and section GC paper over those problems. However, in the end I don't think it makes sense to worry about unmeasured, hypothetical code size problems. If there are problems, I guess it would make sense to factor out "dumping" functionanlity from the PDB and CodeView libraries into their own CVDump helper library or something. This is similar to how we pulled YAML writing out of lib/Object.



================
Comment at: llvm/tools/llvm-pdbutil/InputFile.cpp:40
 getModuleDebugStream(PDBFile &File, StringRef &ModuleName, uint32_t Index) {
-  ExitOnError Err("Unexpected error: ");
-
-  auto &Dbi = Err(File.getPDBDbiStream());
+  auto DbiOrErr = File.getPDBDbiStream();
+  if (!DbiOrErr)
----------------
aganea wrote:
> Even though `auto` is already used throughout this file, I find it's hard for a person reading the code to understand what type the function returns. Also please see https://llvm.org/docs/CodingStandards.html#use-auto-type-deduction-to-make-code-more-readable
> 
> It's probably better if we wrote `Expected<DbiStream &> DbiOrErr = ...` and `DbiStream &Dbi = ...` below. Same applies for other places/usage of `auto` throughout this patch.
I wanted to ask if we can write `Expected<auto>`, but no, that is not valid C++. :(

I don't think I can make a strong argument for either direction. Fully writing out all Expected types seems onerous, but this also seems like an overuse of auto. The type that the reader cares about most is probably DbiStream.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121801