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

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Mar 16 11:00:17 PDT 2022


dblaikie added inline comments.


================
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)
----------------
rnk wrote:
> 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.
I'm mostly OK with using auto for Expected - especially if you're going to name the extracted value, as is done here (`Dbi`) - if you want to include the type name for that for readability, you could include it there (`DbiStream &Dbi = *DbiOrErr`). To me - the Expected's pratical use/scope is 4 lines, and the variable name and its 3 uses all communicate, to me at least, pretty clearly what it is/how it's being used.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121801



More information about the llvm-commits mailing list