[PATCH] D52033: [GCOV] Add options to filter files which must be instrumented.

[Vedant Kumar via Phabricator via llvm-commits]

vsk added inline comments.


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:102
+  bool isFunctionInstrumented(const Function &F);
+  void getRegex(StringRef Filter, std::vector<Regex> &Regexs);
+  static bool isFilenameMatchingRegex(const StringRef &Filename,
----------------
Just reading the signature here, it's not clear what getRegex does. Perhaps a more specific name would help? Maybe 'createRegexesFromFilterString'?

Also, it's an llvm convention to rely on NRVO instead of having a single out-parameter (so, simply returning the std::vector here is fine).


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:103
+  void getRegex(StringRef Filter, std::vector<Regex> &Regexs);
+  static bool isFilenameMatchingRegex(const StringRef &Filename,
+                                      std::vector<Regex> &Regexs);
----------------
StringRef is cheap to copy, no need for a const reference here.


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:135
+  std::vector<Regex> ExcludeRe;
+  StringMap<bool> InstrumentedFiles;
 };
----------------
StringMap<bool> sounds like it's just StringSet?


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:451
+
+bool GCOVProfiler::isFilenameMatchingRegex(const StringRef &Filename,
+                                           std::vector<Regex> &Regexs) {
----------------
Nit, maybe 'doesFilenameMatchARegex' would be a clearer name.


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:476
+  const char *RealPath = nullptr;
+#endif
+
----------------
This might not be portable. Try llvm::sys::fs::real_path.


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:479
+  const StringRef RealFilename = RealPath ? RealPath : Filename;
+  bool Ret;
+  if (FilterRe.empty()) {
----------------
Use a more specific name, perhaps 'bool ShouldInstrument'.


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:487
+          !isFilenameMatchingRegex(RealFilename, ExcludeRe);
+  }
+  InstrumentedFiles[Filename] = Ret;
----------------
Does this do the right thing if both sets of filters are empty?

Stepping back a bit, what is the motivation for having both inclusion & exclusion filters? Why not just have one?

Is it so that you can, say, exclude "/usr/*", but also include "/usr/local/*"? In that case the user needs to know the order in which you apply filters, because they're not commutative.


Repository:
  rL LLVM

https://reviews.llvm.org/D52033