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

[calixte via Phabricator via llvm-commits]

calixte added inline comments.


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:135
+  std::vector<Regex> ExcludeRe;
+  StringMap<bool> InstrumentedFiles;
 };
----------------
vsk wrote:
> StringMap<bool> sounds like it's just StringSet?
I need to know if the filename has been already processed to avoid to do everything again.


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:476
+  const char *RealPath = nullptr;
+#endif
+
----------------
vsk wrote:
> This might not be portable. Try llvm::sys::fs::real_path.
Thx for that, I looked after it and didn't find it.


================
Comment at: lib/Transforms/Instrumentation/GCOVProfiling.cpp:487
+          !isFilenameMatchingRegex(RealFilename, ExcludeRe);
+  }
+  InstrumentedFiles[Filename] = Ret;
----------------
vsk wrote:
> 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.
If both are empty then we returned at the beginning of the function.

Having only one is quite difficult to use.
The 2 common cases I thought about:
 - exclude /usr/include/ stuff
 - include only foo.cpp, bar.cpp (to make code coverage on modified files in a patch)
Having only one will induce that it will be more difficult to write regex.

When we've both then a file is included when it matches one of the inc regex and when it doesn't match all the exc regex.
So exc /usr/* and inc /usr/local/* means that all files in /usr/ are rejected, if you want only the /usr/local/ just don't put exc stuff.
And exc /usr/local/* and inc /usr/* means that we take all files in /usr except the ones in /usr/local.
But definitely, I must make a clearer doc.


Repository:
  rL LLVM

https://reviews.llvm.org/D52033