[PATCH] D63672: Added Delta IR Reduction Tool

[David Blaikie via Phabricator via llvm-commits]

dblaikie added inline comments.


================
Comment at: llvm/tools/llvm-reduce/TestRunner.cpp:7
+                       StringRef InputFilename, SmallString<128> TmpDir)
+    : TestName(TestFilename), TestArgs(TArgs), ReducedFilepath(InputFilename),
+      TmpDirectory(TmpDir) {}
----------------
I'd encourage naming the parameters (both in the ctor definition here, but also in the ctor declaration) the same as the members to avoid confusion. ("do these things represent the same concept? Are they massaged/modified in some way in the ctor implementation to get to the member values?)


================
Comment at: llvm/tools/llvm-reduce/llvm-reduce.cpp:57
+// Parses IR into a Module and verifies it
+std::unique_ptr<Module> parseInputFile(StringRef Filename, LLVMContext &Ctxt) {
+  SMDiagnostic Err;
----------------
dblaikie wrote:
> Probably should be 'static' (as well as other utility functions in this (& any other .cpp) files) if this isn't used outside this file?
Technically the LLVM style guide advocates for using "static" rather than anonymous namespaces (except for classes, where "static" isn't supported by the language): https://llvm.org/docs/CodingStandards.html#anonymous-namespaces


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63672