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

[Bob Haarman via Phabricator via llvm-commits]

inglorion added inline comments.


================
Comment at: llvm/tools/llvm-pdbdump/llvm-pdbdump.cpp:901
+
+  if (opts::shared::DumpModuleSyms || opts::shared::DumpModuleFiles)
+    opts::shared::DumpModules = true;
----------------
zturner wrote:
> inglorion wrote:
> > That takes care of some of the scattering of the code, but most of the code for pdb2yaml::All is still in YAMLOutput.cpp. Did you intend to move that here, too?
> No because none of that is related to the shared options.  You only mentioned handling the shared options in a common place.
Apologies if I did not express myself clearly. What I meant is that the implementation of -all is scattered. For raw, we have this block here, and the block a few lines down under if (opts::RawSubcommand). For pdb2yaml, we have this block here, and the code in YAMLOutputStyle::dump in YAMLOutputStyle.cpp. So there are two places where we check opts::raw::RawAll and do something, and two places where we check opts::pdb2yaml::All and do something. My proposal was intended to bring that down to one place for opts::raw::RawAll and one for opts::pdb2yaml::All.

Alternatively, I would also be ok if you moved all the code for handing both opts::raw::RawAll and opts::pdb2yaml::All here. That's what I thought you were doing, but I see now that it wasn't.

I have a slight preference for the first way, but the second way is fine with me, too. The main thing I want is to have neither pdb2yaml::All or raw::RawAll broken up into pieces that are far apart.


https://reviews.llvm.org/D33996