[PATCH] D63672: Added Delta IR Reduction Tool

[Diego Treviño via Phabricator via llvm-commits]

diegotf added inline comments.


================
Comment at: llvm/tools/llvm-reduce/TestRunner.cpp:46-47
+
+  StringRef SR = "";
+  Optional<StringRef> Redirects[3] = {SR, SR, SR}; // STDIN, STDOUT, STDERR
+  int Result = sys::ExecuteAndWait(TestName, ProgramArgs, None, Redirects);
----------------
dblaikie wrote:
> Do you need to initialize these elements? I suspect just this:
> 
>   Optional<StringRef> Redirects[3];
> 
> should suffice (since they're "Optional" - it'd be weird to have to make them "non-None but empty strings").
Yeah I'm actually not sure why I initialized them 🤔 


================
Comment at: llvm/tools/llvm-reduce/deltas/Delta.cpp:168-169
+    // Delete uninteresting chunks
+    auto UnwantedChunks = Chunks.end();
+    UnwantedChunks = std::remove_if(Chunks.begin(), Chunks.end(),
+                                    [UninterestingChunks](const Chunk &C) {
----------------
dblaikie wrote:
> Looks like an unnecessary initialization? Coeuld be written as:
> 
>   auto UnwantedChunks = std::remove_if....
> 
> Though some people would actually wrap up erase and remove together:
> 
>   Chunks.erase(llvm::remove_if(Chunks, [&](const Chunk &C) {
>     return UninterestingChunks.count(C);
>   }, Chunks.end());
> 
> Oh, there we go, we have an llvm::erase_if:
> 
>   erase_if(Chunks, [&](const Chunk &C) {
>     ...
>   });
> 
> Nice & easy :)
Wow, love the solution. I didn't know `llvm::erase_if` existed, I really need to read up more on some parts of the LLVM API.

Thanks for the cleaner solution!


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D63672