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

Adding --method-blocklist <filepath> parameter to skip renaming of IDL methods.

BUG=673039
Review-Url: https://codereview.chromium.org/2627003003
Cr-Commit-Position: refs/heads/master@{#444205}
Committed: https://chromium.googlesource.com/chromium/src/+/f2ee38a98ac32d112cb9092bb66ac0164adbb92c

[Łukasz Anforowicz, 2017-01-11 21:13:47]

lukasza@chromium.org changed reviewers:
+ dcheng@chromium.org

[Łukasz Anforowicz, 2017-01-11 21:13:48]

Daniel, can you PTAL?

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

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:85: class
IdlMatcherState {
Nit: MethodBlocklist

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:91: //    should be
okay / shouldn't conflict with real Blink methods.
Perhaps test_tool.py should understand how to parse an args.txt file and pass it
as --tool-args?

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:98: bool
IsMatching(const clang::CXXMethodDecl& method) const {
Nit: Contains / Matches / Has

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:142: std::ifstream
stream(filepath);
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.

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:160:
llvm::SmallVector<llvm::StringRef, kExpectedNumberOfParts + 1> parts;
Why + 1?

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:178: << ":" <<
line_number << ": " << line << "\n";
Should we make errors fatal?

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:190:
std::map<std::string, std::map<std::string, std::set<int>>>
Perhaps consider StringMap<StringMap<std::set<int>>> here, which should largely
be a drop-in replacement, but slightly more efficient.

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:201:
clang::ast_matchers::internal::Matcher<clang::CXXMethodDecl> idlMethod(
idlMethod => isBlocklistedMethod per the other nits

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1160:
llvm::cl::desc("file listing method names derived from Web IDL"));
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]

Thanks for the feedback.  Can you take another look please?

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:85: class
IdlMatcherState {
On 2017/01/12 11:00:58, dcheng wrote:
> Nit: MethodBlocklist

Done.

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:91: //    should be
okay / shouldn't conflict with real Blink methods.
On 2017/01/12 11:00:58, dcheng wrote:
> Perhaps test_tool.py should understand how to parse an args.txt file and pass
it
> as --tool-args?

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).

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:98: bool
IsMatching(const clang::CXXMethodDecl& method) const {
On 2017/01/12 11:00:58, dcheng wrote:
> Nit: Contains / Matches / Has

Done (Contains).

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:142: std::ifstream
stream(filepath);
On 2017/01/12 11:00:58, dcheng wrote:
> 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.

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.

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:160:
llvm::SmallVector<llvm::StringRef, kExpectedNumberOfParts + 1> parts;
On 2017/01/12 11:00:58, dcheng wrote:
> Why + 1?

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.

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:174: int
number_of_method_args;
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]).

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:178: << ":" <<
line_number << ": " << line << "\n";
On 2017/01/12 11:00:59, dcheng wrote:
> Should we make errors fatal?

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).

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:190:
std::map<std::string, std::map<std::string, std::set<int>>>
On 2017/01/12 11:00:58, dcheng wrote:
> Perhaps consider StringMap<StringMap<std::set<int>>> here, which should
largely
> be a drop-in replacement, but slightly more efficient.

Good point.  Done.

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:201:
clang::ast_matchers::internal::Matcher<clang::CXXMethodDecl> idlMethod(
On 2017/01/12 11:00:58, dcheng wrote:
> idlMethod => isBlocklistedMethod per the other nits

Done.

https://codereview.chromium.org/2627003003/diff/40001/tools/clang/rewrite_to_...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:1160:
llvm::cl::desc("file listing method names derived from Web IDL"));
On 2017/01/12 11:00:58, dcheng wrote:
> Nit: let's use a more generic name to indicate what it does. I'd suggest
> method-blocklist.

Done.

[Łukasz Anforowicz, 2017-01-13 21:55:24]

Description was changed from

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

BUG=673039
==========

to

==========
Adding --method-blocklist <filepath> parameter to skip renaming of IDL methods.

BUG=673039
==========

[Łukasz Anforowicz, 2017-01-13 21:56:01]

FYI - I've added handling of optional args as we've discussed in the hangout. 
Can you PTAL?

[dcheng, 2017-01-17 22:56:11]

LGTM with nits

https://codereview.chromium.org/2627003003/diff/120001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2627003003/diff/120001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:142:
assert(actual_class);
Nit: a lot of times, assert messages in LLVM look like:

assert(actual_class && "some descriptive message here");

It might be worth considering doing that here.

https://codereview.chromium.org/2627003003/diff/120001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:149: const
std::set<unsigned>& set_of_expected_number_of_args = it2->second;
Nit: from the context, it's not clear if expected means blocked or unblocked. I
would just name this something sort like |arg_counts| (which matches how things
like class_to_args and method_to_class_to_args are named.

I'm having a trouble keeping expected / actual straight as I read this function.

https://codereview.chromium.org/2627003003/diff/120001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:156: bool
got_expected_number_of_args = std::any_of(
Nit: Rename got_expected_to_number_of_args => found_matching_arg_count?

https://codereview.chromium.org/2627003003/diff/120001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:172: return true;
Then just return found_matching_arg_count directly here (we can keep the TODO)

[dcheng, 2017-01-17 22:59:50]

https://codereview.chromium.org/2627003003/diff/120001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2627003003/diff/120001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:241:
MethodBlocklist(method_blocklist_filepath));
Oh, one other thing: do we need to split this between an internal / non-internal
matcher? Can line 1555 just construct the MethodBlocklist object and pass it in
directly?

[Łukasz Anforowicz, 2017-01-17 23:52:47]

https://codereview.chromium.org/2627003003/diff/120001/tools/clang/rewrite_to...
File tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp (right):

https://codereview.chromium.org/2627003003/diff/120001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:142:
assert(actual_class);
On 2017/01/17 22:56:10, dcheng wrote:
> Nit: a lot of times, assert messages in LLVM look like:
> 
> assert(actual_class && "some descriptive message here");
> 
> It might be worth considering doing that here.

Done.

https://codereview.chromium.org/2627003003/diff/120001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:149: const
std::set<unsigned>& set_of_expected_number_of_args = it2->second;
On 2017/01/17 22:56:10, dcheng wrote:
> Nit: from the context, it's not clear if expected means blocked or unblocked.
I
> would just name this something sort like |arg_counts| (which matches how
things
> like class_to_args and method_to_class_to_args are named.
> 
> I'm having a trouble keeping expected / actual straight as I read this
function.

Done.

https://codereview.chromium.org/2627003003/diff/120001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:156: bool
got_expected_number_of_args = std::any_of(
On 2017/01/17 22:56:11, dcheng wrote:
> Nit: Rename got_expected_to_number_of_args => found_matching_arg_count?

Done.

https://codereview.chromium.org/2627003003/diff/120001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:172: return true;
On 2017/01/17 22:56:10, dcheng wrote:
> Then just return found_matching_arg_count directly here (we can keep the TODO)

Done.

https://codereview.chromium.org/2627003003/diff/120001/tools/clang/rewrite_to...
tools/clang/rewrite_to_chrome_style/RewriteToChromeStyle.cpp:241:
MethodBlocklist(method_blocklist_filepath));
On 2017/01/17 22:59:50, dcheng wrote:
> Oh, one other thing: do we need to split this between an internal /
non-internal
> matcher? Can line 1555 just construct the MethodBlocklist object and pass it
in
> directly?

Done.  I did it this way thinking that one day we'll move
isBlocklistedMethod(filename) to a separate header (without exposing
MethodBlocklist class through the header), but you're right that it doesn't make
sense today.

[Łukasz Anforowicz, 2017-01-17 23:52:52]

The CQ bit was checked by lukasza@chromium.org

[Łukasz Anforowicz, 2017-01-17 23:52:52]

The patchset sent to the CQ was uploaded after l-g-t-m from dcheng@chromium.org
Link to the patchset: https://codereview.chromium.org/2627003003/#ps140001
(title: "Addressed remaining CR feedback from dcheng@.")

[commit-bot: I haz the power, 2017-01-17 23:53:22]

CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[Łukasz Anforowicz, 2017-01-18 00:22:47]

Daniel, can you PTAL?

[Łukasz Anforowicz, 2017-01-18 00:23:23]

On 2017/01/18 00:22:47, Łukasz Anforowicz wrote:
> Daniel, can you PTAL?

Ooops.  Wrong CL... :-(  Please ignore the message above.

[commit-bot: I haz the power, 2017-01-18 00:36:39]

CQ is committing da patch.

Bot data: {"patchset_id": 140001, "attempt_start_ts": 1484697172548020,
"parent_rev": "a8ad82c005ae878c448ae500b3f7b5e3a880ad56", "commit_rev":
"f2ee38a98ac32d112cb9092bb66ac0164adbb92c"}

[commit-bot: I haz the power, 2017-01-18 00:37:16]

Description was changed from

==========
Adding --method-blocklist <filepath> parameter to skip renaming of IDL methods.

BUG=673039
==========

to

==========
Adding --method-blocklist <filepath> parameter to skip renaming of IDL methods.

BUG=673039

Review-Url: https://codereview.chromium.org/2627003003
Cr-Commit-Position: refs/heads/master@{#444205}
Committed:
https://chromium.googlesource.com/chromium/src/+/f2ee38a98ac32d112cb9092bb66a...
==========

[commit-bot: I haz the power, 2017-01-18 00:37:17]

Committed patchset #8 (id:140001) as
https://chromium.googlesource.com/chromium/src/+/f2ee38a98ac32d112cb9092bb66a...