Add --idl-methods <filepath> parameter for skipping renaming of IDL methods.

tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp

Index: tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
diff --git a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
index c3d3356d2542a138cf35bdd3f4e086eb357d4983..c2993baf4473bb3543f00ef116d20caca7478b18 100644
--- a/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
+++ b/tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp
@@ -14,8 +14,10 @@
 
 #include <assert.h>
 #include <algorithm>
+#include <fstream>
 #include <memory>
 #include <set>
+#include <sstream>
 #include <string>
 
 #include "clang/AST/ASTContext.h"
@@ -44,6 +46,7 @@ namespace {
 const char kBlinkFieldPrefix[] = "m_";
 const char kBlinkStaticMemberPrefix[] = "s_";
 const char kGeneratedFileRegex[] = "^gen/|/gen/";
+const char kIdlMethodsParamName[] = "idl-methods";
 
 template <typename MatcherType, typename NodeType>
 bool IsMatching(const MatcherType& matcher,
@@ -79,6 +82,127 @@ AST_MATCHER_P(clang::FunctionTemplateDecl,
   return InnerMatcher.matches(*Node.getTemplatedDecl(), Finder, Builder);
 }
 
+class IdlMatcherState {

[dcheng, 2017/01/12 11:00:58]

Nit: MethodBlocklist

[Łukasz Anforowicz, 2017/01/12 23:32:01]

Done.

+ public:
+  explicit IdlMatcherState(const std::string& filepath) {
+    if (filepath.empty()) {
+      // 1. test_tool.py will run the clang tool without --idl-methods argument.
+      // 2. If --idl-methods is empty in a normal run, the test-only entries
+      //    should be okay / shouldn't conflict with real Blink methods.

[dcheng, 2017/01/12 11:00:58]

Perhaps test_tool.py should understand how to parse an args.txt file and pass it
as --tool-args?

[Łukasz Anforowicz, 2017/01/12 23:32:01]

Done.

test_tool.py will look for a file named run_tool.args in the tool test directory
and treat each line as an argument to add to run_tool.py invocation.

test_tool.py will also switch to the tool test directory before invoking
run_tool.py (this helps specify relative paths in run_tool.args).

+      SetupTestBlacklist();
+    } else {
+      ParseInputFile(filepath);
+    }
+  }
+
+  bool IsMatching(const clang::CXXMethodDecl& method) const {

[dcheng, 2017/01/12 11:00:58]

Nit: Contains / Matches / Has

[Łukasz Anforowicz, 2017/01/12 23:32:01]

Done (Contains).

+    auto it = method_to_class_to_args_.find(method.getName());
+    if (it == method_to_class_to_args_.end())
+      return false;
+
+    const clang::CXXRecordDecl* actual_class = method.getParent();
+    // Hopefully |getParent()| can find the class even for non-inlined method
+    // definitions (where CXXRecordDecl is not a parent AST node of
+    // CXXMethodDecl).
+    assert(actual_class);
+
+    const std::map<std::string, std::set<int>>& class_to_args = it->second;
+    auto it2 = class_to_args.find(actual_class->getName());
+    if (it2 == class_to_args.end())
+      return false;
+
+    std::set<int> expected_number_of_args = it2->second;
+    int actual_number_of_args = method.param_size();
+    if (0 == expected_number_of_args.count(-1) &&
+        0 == expected_number_of_args.count(actual_number_of_args))
+      return false;
+
+    // No need to verify here that |actual_class| is in the |blink| namespace -
+    // this will be done by other matchers elsewhere.
+
+    // TODO(lukasza): Do we need to consider return type and/or param types?
+
+    return true;
+  }
+
+ private:
+  void SetupTestBlacklist() {
+    std::istringstream stream(
+        "IdlTestClass:::idlTestMethodNoParams:::0\n"
+        "IdlTestClass:::idlTestMethodOneParam:::1\n"
+        "IdlTestClass:::idlTestMethodTwoOrThreeParams:::2\n"
+        "IdlTestClass:::idlTestMethodTwoOrThreeParams:::3\n"
+        "IdlTestClass:::idlTestMethodAnyNumberOfParams:::-1\n"
+        "IdlTestClass:::path:::0\n");  // "Get" prefix test.
+
+    ParseInputStream("<test input>", stream);
+  }
+
+  void ParseInputFile(const std::string& filepath) {
+    std::ifstream stream(filepath);

[dcheng, 2017/01/12 11:00:58]

Since this is LLVM/clang code, let's go with the LLVMisms:

https://cs.chromium.org/chromium/src/third_party/llvm/lib/Object/ArchiveWrite...
is an example of how to use openFileForRead and combine it with
MemoryBuffer::getOpenFile() to map it in.

Then we can use line_iterator
(https://cs.chromium.org/chromium/src/third_party/llvm/include/llvm/Support/Li...)
to read the files and go through the rest of this processing.

[Łukasz Anforowicz, 2017/01/12 23:32:01]

Done.

I like how I automagically/effortlessly got support for skipping blank lines and
comment lines.  And for tracking the current line number.

The iterator interface feels weird / I couldn't figure out how to use this with
a range-based for loop.

I am not sure if the file contents are all read into memory (vs is the file
streamed).  I guess this doesn't matter much - the contents of the file will end
up in memory one way or another.

+    ParseInputStream(filepath, stream);
+  }
+
+  void ParseInputStream(const std::string& filepath, std::istream& stream) {
+    if (stream.fail()) {
+      llvm::errs() << "ERROR: Cannot open the file specified in --"
+                   << kIdlMethodsParamName << " argument: " << filepath << "\n";
+      return;
+    }
+
+    // Each line is expected to have the following format:
+    // <class name>:::<method name>:::<number of arguments>
+    std::string line;
+    int line_number = 0;
+    while (std::getline(stream, line)) {
+      // Split the line into ':::'-delimited parts.
+      const size_t kExpectedNumberOfParts = 3;
+      llvm::SmallVector<llvm::StringRef, kExpectedNumberOfParts + 1> parts;

[dcheng, 2017/01/12 11:00:58]

Why + 1?

[Łukasz Anforowicz, 2017/01/12 23:32:01]

You are right, I should remove it.  I tried to be too smart - future proofing /
safe fallback in case there are more entries in a future version of the format. 
We should fail in this case, not be smart.

Done.

+      llvm::StringRef(line).split(parts, ":::");
+      line_number++;
+      if (parts.size() < kExpectedNumberOfParts) {
+        llvm::errs() << "ERROR: Parsing error - less than "
+                     << kExpectedNumberOfParts
+                     << " ':::'-delimited parts: " << filepath << ":"
+                     << line_number << ": " << line << "\n";
+        continue;
+      }
+
+      // Parse individual parts.
+      llvm::StringRef class_name = parts[0];
+      llvm::StringRef method_name = parts[1];
+      int number_of_method_args;

[Łukasz Anforowicz, 2017/01/12 23:32:01]

I stopped allowing -1 here as a way to say "any number of parameters" (I learned
that variadic parameters in IDL map to a single C++ parameter and because of
that I stopped emitting -1 in the blocklist generator [in the parallel CL]).

+      if (parts[2].getAsInteger(0, number_of_method_args)) {
+        llvm::errs() << "ERROR: Parsing error - "
+                     << "'" << parts[2] << "' is not an integer: " << filepath
+                     << ":" << line_number << ": " << line << "\n";

[dcheng, 2017/01/12 11:00:59]

Should we make errors fatal?

[Łukasz Anforowicz, 2017/01/12 23:32:01]

Done.

I am not sure if asserts are the right way to trigger a fatal error, but we use
them everywhere else here...  Maybe something to consider changing in a separate
CL (not sure how much of a performance boost we would get in a release build of
the clang tool).

+        continue;
+      }
+
+      // Store the new entry.
+      method_to_class_to_args_[method_name][class_name].insert(
+          number_of_method_args);
+    }
+  }
+
+  // Stores methods to blacklist in a map:
+  // method name -> class name -> set of all allowed numbers of arguments.
+  std::map<std::string, std::map<std::string, std::set<int>>>

[dcheng, 2017/01/12 11:00:58]

Perhaps consider StringMap<StringMap<std::set<int>>> here, which should largely
be a drop-in replacement, but slightly more efficient.

[Łukasz Anforowicz, 2017/01/12 23:32:01]

Good point.  Done.

+      method_to_class_to_args_;
+};
+
+AST_MATCHER_P(clang::CXXMethodDecl,
+              internalIdlMethod,
+              IdlMatcherState,
+              MatcherState) {
+  return MatcherState.IsMatching(Node);
+}
+
+clang::ast_matchers::internal::Matcher<clang::CXXMethodDecl> idlMethod(

[dcheng, 2017/01/12 11:00:58]

idlMethod => isBlocklistedMethod per the other nits

[Łukasz Anforowicz, 2017/01/12 23:32:01]

Done.

+    const std::string& idl_methods_filepath) {
+  return internalIdlMethod(IdlMatcherState(idl_methods_filepath));
+}
+
 // If |InnerMatcher| matches |top|, then the returned matcher will match:
 // - |top::function|
 // - |top::Class::method|
@@ -1031,6 +1155,9 @@ int main(int argc, const char* argv[]) {
   llvm::InitializeNativeTargetAsmParser();
   llvm::cl::OptionCategory category(
       "rewrite_to_chrome_style: convert Blink style to Chrome style.");
+  llvm::cl::opt<std::string> idl_methods_file(
+      kIdlMethodsParamName, llvm::cl::value_desc("filepath"),
+      llvm::cl::desc("file listing method names derived from Web IDL"));

[dcheng, 2017/01/12 11:00:58]

Nit: let's use a more generic name to indicate what it does. I'd suggest
method-blocklist.

[Łukasz Anforowicz, 2017/01/12 23:32:01]

Done.

   CommonOptionsParser options(argc, argv, category);
   clang::tooling::ClangTool tool(options.getCompilations(),
                                  options.getSourcePathList());
@@ -1182,7 +1309,8 @@ int main(int argc, const char* argv[]) {
   // we use includeAllOverriddenMethods() to check these rules not just for the
   // method being matched but for the methods it overrides also.
   auto is_blink_method = includeAllOverriddenMethods(
-      allOf(in_blink_namespace, unless(isBlacklistedMethod())));
+      allOf(in_blink_namespace, unless(isBlacklistedMethod()),
+            unless(idlMethod(idl_methods_file))));
   auto method_decl_matcher = id(
       "decl",
       cxxMethodDecl(