[PATCH] D33996: Improve consistency and flexibility with llvm-pdbdump subcommands

[Bob Haarman via Phabricator via llvm-commits]

inglorion added a comment.

Thanks for doing this, zturner! I have some more inline comments and questions.



================
Comment at: llvm/test/DebugInfo/PDB/pdbdump-readwrite.test:1
-RUN: llvm-pdbdump pdb2yaml -dbi-module-info -dbi-module-source-info \
-RUN:   -dbi-stream -pdb-stream -string-table -tpi-stream -stream-directory \
+RUN: llvm-pdbdump pdb2yaml -modules -module-files -dbi-stream \
+RUN:   -pdb-stream -string-table -tpi-stream -stream-directory \
----------------
While thinking about this: What does -dbi-stream mean? Is it useful to specify -dbi-stream without any of the additional enabling flags like -modules?

Also, should we at some point change this test to use -all instead of the various individual options?


================
Comment at: llvm/tools/llvm-pdbdump/YAMLOutputStyle.cpp:47
     opts::pdb2yaml::DbiStream = true;
-    opts::pdb2yaml::DbiModuleInfo = true;
-    opts::pdb2yaml::DbiModuleSyms = true;
-    opts::pdb2yaml::DbiModuleSourceFileInfo = true;
-    opts::pdb2yaml::DbiModuleSourceLineInfo = true;
+    // opts::pdb2yaml::DbiModuleSourceLineInfo = true;
     opts::pdb2yaml::TpiStream = true;
----------------
Please remove the commented-out code (also further down).


================
Comment at: llvm/tools/llvm-pdbdump/YAMLOutputStyle.cpp:124
 Error YAMLOutputStyle::dumpStringTable() {
-  bool RequiresStringTable = opts::pdb2yaml::DbiModuleSourceFileInfo ||
-                             opts::pdb2yaml::DbiModuleSourceLineInfo;
+  bool RequiresStringTable = opts::shared::DumpModuleFiles ||
+                             !opts::shared::DumpModuleSubsections.empty();
----------------
Since you're changing this - would it make sense to rewrite this

  if (!opts::pdb2yaml::StringTable && !opts::shared::DumpModuleFiles && opts::shared::DumpModuleSubsections.empty()))
    return Error::success();

?


================
Comment at: llvm/tools/llvm-pdbdump/llvm-pdbdump.cpp:887
 
+  if ((opts::RawSubcommand && opts::raw::RawAll) ||
+      (opts::PdbToYamlSubcommand && opts::pdb2yaml::All)) {
----------------
Can you make a function that enables all shared options and call that from the places where we handle -all for pdb2yaml and raw? That way, the code that handles -all for pdb2yaml is all in one place, and the code that handles -all for raw is all in one place, instead of both being partially here and partially where they are now.


https://reviews.llvm.org/D33996