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

[Zachary Turner via Phabricator via llvm-commits]

zturner marked an inline comment as done.
zturner added inline comments.


================
Comment at: llvm/tools/llvm-pdbdump/llvm-pdbdump.cpp:901
+
+  if (opts::shared::DumpModuleSyms || opts::shared::DumpModuleFiles)
+    opts::shared::DumpModules = true;
----------------
inglorion wrote:
> 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.
Ok, I can do that.  I think the second way might be easier, just because pdb2yaml All and raw All have some subtle differences.

In the future, if I go forwad with the idea to change raw to be more compressed for human readability, then we can abandon the requirement for the output to map closely to the file format (since we have pdb2yaml for that), and then a lot of the option decisions can be simplified I think.


https://reviews.llvm.org/D33996