[PATCH] D86974: [IRSim] Adding basic implementation of llvm-sim.

[Jon Roelofs via Phabricator via llvm-commits]

jroelofs added inline comments.


================
Comment at: llvm/tools/llvm-sim/FindSimilarities.cpp:50
+  // case.
+  if (!sys::fs::is_regular_file(Status)) {
+    error() << "ignoring " << Path
----------------
A useful pattern for tools like this (and I think this might forbid?) is to support reading from stdin / writing to stdout when the filename is `-`.  That's also useful for the tests, because it lets you eliminate the `cat %t` RUN lines. 


================
Comment at: llvm/tools/llvm-sim/FindSimilarities.h:27
+public:
+  SimilarityFinderTool(raw_ostream &OS, std::string &Path);
+  ~SimilarityFinderTool();
----------------
StringRef?


================
Comment at: llvm/tools/llvm-sim/FindSimilarities.h:58
+  /// \returns nonzero if there was an error finding the \p Path.
+  int collectPath(const std::string &Path);
+
----------------
StringRef?


================
Comment at: llvm/tools/llvm-sim/JSONExporter.h:44
+  /// \returns A nonzero error code if there was a failure creating the file.
+  int exportToFile(const std::string &FilePath,
+                   const SimilarityGroupList &SimSections,
----------------
A free function would probably be fine. The class doesn't seem to be buying you much here.


================
Comment at: llvm/tools/llvm-sim/llvm-sim.cpp:36
+
+  SimilarityFinderTool SimTool(errs(), InputSourceFile);
+  int E = SimTool.getFailure();
----------------
I think it would clean up the error handling a bit if the file handling & bitcode reading all happened here, and then have this transfer ownership of the constructed module into the tool object.

That said, the tool object isn't buying you much either. I'd probably end up folding most/all of it into this file, with helper functions as needed.


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

https://reviews.llvm.org/D86974