[PATCH] D37624: add support for -fno-instrument-functions and -finstrument-functions-exclude-{file, function}-list=<arg1, arg2, ...> to match gcc options.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:425
 /// instrumented with __cyg_profile_func_* calls
-bool CodeGenFunction::ShouldInstrumentFunction() {
+bool CodeGenFunction::ShouldInstrumentFunction(llvm::Function *Fn) {
+  typedef std::vector<std::string>::const_iterator CIt;
----------------
This parameter can be `const`.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:436
+  // to resolve.
+  const FunctionDecl *ActualFuncDecl = dyn_cast<FunctionDecl>(CurFuncDecl);
+  if (ActualFuncDecl &&
----------------
Can use `const auto *` here.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:444
+  SourceLocation SLoc = CurFuncDecl->getLocation();
+  static std::unordered_map<unsigned, std::string> cache;
+
----------------
This static has me worried. Does this data need to be cached across codegen modules? If not, perhaps this variable can be hung onto the CodeGenModule instead?

The variable should be named `Cache` instead of `cache`. Also, is an `unordered_map` the correct data structure to use? Would a `DenseMap` make more sense because the keys and values are both small (`PresumedLoc::getFilename()` returns a `const char *` that I believe can be used).


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:447
+  if (SLoc.isFileID()) {
+    unsigned key = SLoc.getRawEncoding();
+    if (cache.find(key) == cache.end()) {
----------------
key -> Key


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:449
+    if (cache.find(key) == cache.end()) {
+      ASTContext &ctx = CurFuncDecl->getASTContext();
+      const SourceManager &SM = ctx.getSourceManager();
----------------
ctx -> CTX, and I think this can be a const ref.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:460-461
+
+    for (CIt i = PathSearch.begin(), e = PathSearch.end(); i != e; ++i) {
+      if(FunctionDeclPath.find(*i) != std::string::npos) {
+        return false;
----------------
You can use a range-based for loop here instead, and then get rid of the typedef for `CIt`.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:467
+
+  std::string FunctionName = Fn->getName();
+
----------------
You can avoid the copy here by assigning to a `StringRef` instead.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:472
+  if (ActualFuncDecl && !ActualFuncDecl->isExternC()) {
+    int status = 0;
+    char *result = __cxa_demangle(FunctionName.c_str(), 0, 0, &status);
----------------
status -> Status


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:473
+    int status = 0;
+    char *result = __cxa_demangle(FunctionName.c_str(), 0, 0, &status);
+
----------------
result -> Result


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:486
+    CGM.getCodeGenOpts().InstrumentFunctionExclusionsFunctions;
+  for (CIt i = FunctionSearch.begin(), e = FunctionSearch.end(); i != e; ++i) {
+    if(FunctionName.find(*i) != std::string::npos) {
----------------
Can use a range-based for loop here.


https://reviews.llvm.org/D37624