[PATCH] D63672: Added Delta IR Reduction Tool

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

I'd suggest enhancing the main description to include an overview of the code structure and organization to help reviewers follow the implementation design here. Think of it like a mini design doc for the *implementation* itself.



================
Comment at: llvm/test/Reduce/inputs/remove-funcs.sh:1
+#!/bin/sh
+
----------------
The directory is typically capitalized as `Inputs` in LLVM.


================
Comment at: llvm/test/Reduce/remove-funcs.ll:6
+; RUN: cat reduced.ll | FileCheck %s
+; REQUIRES: plugins
+
----------------
You'll also want to say that this requires a shell. While the test itself doesn't it requires one on the system to run the "test" script.


================
Comment at: llvm/tools/llvm-reduce/TestRunner.cpp:7-9
+/// RunProgramWithTimeout - This function provides an alternate interface
+/// to the sys::Program::ExecuteAndWait interface.
+/// @see sys::Program::ExecuteAndWait
----------------
No need to repeat the function names.

But more importantly -- what kind of alternative? What's the difference?


================
Comment at: llvm/tools/llvm-reduce/TestRunner.h:8-12
+//
+// This file contains all the info necessary for running the provided
+// interesting-ness test, as well as the most reduced module and its
+// respective filename.
+//
----------------
I'd move this to be a doxygen comment on the class. You can skip the file comment if its just one class.


================
Comment at: llvm/tools/llvm-reduce/TestRunner.h:33-34
+
+  /// Runs the interesting-ness test for the specified file
+  int run(SmallString<128> Filename);
+
----------------
What does the return value mean?


================
Comment at: llvm/tools/llvm-reduce/TestRunner.h:44
+  void setReducedFilepath(SmallString<128> F) { ReducedFilepath = F; }
+  void setProgram(std::unique_ptr<Module> &P) { Program = std::move(P); }
+
----------------
Typically best to pass `std::unique_ptr` by value.


================
Comment at: llvm/tools/llvm-reduce/TestRunner.h:47
+private:
+  SmallString<128> TestName;
+  std::vector<std::string> TestArgs;
----------------
Any particular reason to use `SmallString` instead of `std::string`?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63672