[PATCH] D65026: [Bugpoint redesign] Added pass to reduce Metadata

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

diegotf added inline comments.


================
Comment at: llvm/tools/llvm-reduce/DeltaManager.h:19-40
+
+namespace {
+static ManagedStatic<std::vector<std::string>> DeltaPasses;
+
+struct DeltaPassOption {
+  void operator=(const std::string &Val) const {
+    if (Val.empty())
----------------
dblaikie wrote:
> Anonymous namespaces and static variables aren't suitable for header files (they can lead to ODR violations, etc) You can inline these things inside runDeltaPasses (using lambdas for local functions, etc) or take the runDeltaPasses definition out of line into a .cc file and move these static variables/anonymous namespace content to there.
I don't know why I keep forgetting this, I swear I'll tattoo it on my arm next time I forget haha.

Thanks for the comment, David.


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceGlobalVars.cpp:15
 
-#include "RemoveGlobalVars.h"
+#include "ReduceGlobalVars.h"
 
----------------
dblaikie wrote:
> Might've been a bit easier to do some of these renames as separate patches - super easy to review (or commit without review) & keeps the "interesting" reviews uncluttered.
My bad, I'll do that from now on :)


================
Comment at: llvm/tools/llvm-reduce/deltas/ReduceMetadata.cpp:20
+static void getChunkMetadataNodes(T *MDUser, int &I,
+                                  std::vector<Chunk> ChunksToKeep,
+                                  std::set<MDNode *> &SeenNodes,
----------------
dblaikie wrote:
> Is this intentionally passed by value for some reason? (usually a non-trivial data structure like a std::vector would be passed by const ref if a copy isn't needed, etc)
Good catch, I forgot to pass it as const ref. 

I'll make another patch later with the same fix on other Delta Passes to reduce the clutter on this diff


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D65026