[PATCH] D54179: [llvm-mca] Move the AssembleInput logic into its own class.

[Clement Courbet via Phabricator via llvm-commits]

courbet added a comment.

I like the patch, I only have cosmetic comments.



================
Comment at: tools/llvm-mca/AsmCodeRegionGenerator.h:49
+  // A comment consumer that parses strings.  The only valid tokens are strings.
+  class MCACommentConsumer : public AsmCommentConsumer {
+  public:
----------------
this can be moved to the cpp file.


================
Comment at: tools/llvm-mca/AsmCodeRegionGenerator.h:57
+
+  class MCStreamerWrapper final : public MCStreamer {
+    CodeRegions &Regions;
----------------
ditto


================
Comment at: tools/llvm-mca/CodeRegion.h:40
 #include "llvm/MC/MCInst.h"
+#include "llvm/Support/CachePruning.h"
 #include "llvm/Support/SMLoc.h"
----------------
why ?


================
Comment at: tools/llvm-mca/CodeRegion.h:130
+/// driver, and converting that into a CodeRegions instance.
+class CodeRegionGenerator {
+protected:
----------------
this is missing a virtual dtor (that you might want to use as anchor BTW).


================
Comment at: tools/llvm-mca/llvm-mca.cpp:344
   llvm::InitializeAllTargetMCs();
-  llvm::InitializeAllAsmParsers();
 
----------------
Most tools do the initialization in the main, I'd rather keep this here rather than in `AsmCodeRegionGenerator::AsmCodeRegionGenerator`


https://reviews.llvm.org/D54179