[PATCH] D101340: Allows for dsymutil crashes to generate reproduceable information

[Jonas Devlieghere via Phabricator via llvm-commits]

JDevlieghere added inline comments.


================
Comment at: llvm/tools/dsymutil/Reproducer.cpp:108
+    Repro = std::make_unique<ReproducerGenerateForCrash>(EC);
+    LLVM_FALLTHROUGH;
+  case ReproducerMode::Generate:
----------------
Why is this a fallthrough? It seems like now, if we fail to create the `ReproducerGenerateForCrash` instance, we'll try creating a regular `ReproducerGenerate` and only then print the error message? I think we should just handle the EC immediately. 


================
Comment at: llvm/tools/dsymutil/Reproducer.h:66-67
 public:
-  ReproducerGenerate(std::error_code &EC);
+  ReproducerGenerate(std::error_code &EC,
+                     ReproducerMode Mode = ReproducerMode::Generate);
   ~ReproducerGenerate() override;
----------------
Do we still need the ability to specify the mode? 


================
Comment at: llvm/tools/dsymutil/Reproducer.h:82
 
+class ReproducerGenerateForCrash : public ReproducerGenerate {
+public:
----------------



================
Comment at: llvm/tools/dsymutil/Reproducer.h:85
+  ReproducerGenerateForCrash(std::error_code &EC);
+  ReproducerGenerateForCrash(ReproducerGenerate &&);
+  void dodump(bool DoDataDump);
----------------
Why do we need a move ctor? 


================
Comment at: llvm/tools/dsymutil/Reproducer.h:95
+private:
+  bool DumpFiles = true;
+};
----------------
I'm not sure if this is really something we want, with any real-world project the number of object files can quickly become overwhelming. Even if we do, this should probably be a property of the `ReproducerGenerate` and not be specific for the crash one. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101340