Add a clang tool to detect std::move(raw ptr)

Add a clang tool to detect std::move(raw ptr)

The CL also adds a sentence to the documentation for clang tools, to supply a
bit which was unclear for me during writing of the added tool.

So far, on Linux, the tool found instances of std::move(raw ptr) in:

../../components/browser_sync/profile_sync_service.cc:10101
../../components/sync_sessions/sessions_sync_manager.cc:5987
../../content/browser/indexed_db/indexed_db_transaction.cc:3865
../../gpu/ipc/service/gpu_channel.cc:26467
../../services/preferences/tracked/pref_hash_store_impl.cc:3329
../../services/preferences/tracked/pref_hash_store_impl.cc:4518
../../services/ui/public/cpp/bitmap/child_shared_bitmap_manager.cc:5009
../../services/video_capture/test/mock_device_factory.cc:2266

It also found it in some 3rd party headers, such as
../../third_party/flatbuffers/src/include/flatbuffers/flatbuffers.h:8260
../../third_party/skia/include/core/SkCanvas.h:14756
../../third_party/webrtc/api/proxy.h:16937
../../third_party/webrtc/api/proxy.h:19681
../../third_party/webrtc/api/proxy.h:20241
and c++ libs.

The Chromium 1st party instances will be addressed in separate CLs. For the headers, I'll consider if it makes sense to address this (e.g., some occurrences seem to be template instantiations, where the template is not a priori aware that the supplied type is a raw pointer; in such cases std::move should stay).

I may also run the tool on more platforms.

Finally, while preventing regressions would be good, it seems impractical to run this tool as a presubmit check. However, I will try to run it after https://crbug.com/646113 is finished, because that base::Value refactoring might introduce some instances (happened to me recently).

BUG=729393, 731577
Review-Url: https://codereview.chromium.org/2919243002
Cr-Commit-Position: refs/heads/master@{#478226}
Committed: https://chromium.googlesource.com/chromium/src/+/9ed3f43a6cc49b844d769498283b2e1e0be6cb94

[vabr (Chromium), 2017-06-04 10:54:59]

Description was changed from

==========
Add a clang tool to detect std::move(raw ptr)

The CL also adds a sentence to the documentation for clang tools, to supply a
bit which was unclear for me during writing of the added tool.

BUG=729393
==========

to

==========
Add a clang tool to detect std::move(raw ptr)

The CL also adds a sentence to the documentation for clang tools, to supply a
bit which was unclear for me during writing of the added tool.

So far, on Linux, the tool found instances of std::move(raw ptr) in:

../../components/browser_sync/profile_sync_service.cc:10101
../../components/sync_sessions/sessions_sync_manager.cc:5987
../../content/browser/indexed_db/indexed_db_transaction.cc:3865
../../gpu/ipc/service/gpu_channel.cc:26467
../../services/preferences/tracked/pref_hash_store_impl.cc:3329
../../services/preferences/tracked/pref_hash_store_impl.cc:4518
../../services/ui/public/cpp/bitmap/child_shared_bitmap_manager.cc:5009
../../services/video_capture/test/mock_device_factory.cc:2266

It also found it in some 3rd party headers, such as
../../third_party/flatbuffers/src/include/flatbuffers/flatbuffers.h:8260
../../third_party/skia/include/core/SkCanvas.h:14756
../../third_party/webrtc/api/proxy.h:16937
../../third_party/webrtc/api/proxy.h:19681
../../third_party/webrtc/api/proxy.h:20241
and c++ libs.

The Chromium 1st party instances will be addressed in separate CLs. For the
headers, I'll consider if it makes sense to address this (e.g., some occurrences
seem to be template instantiations, where the template is not a priori aware
that the supplied type is a raw pointer; in such cases std::move should stay).

I may also run the tool on more platforms.

Finally, while preventing regressions would be good, it seems impractical to run
this tool as a presubmit check. However, I will try to run it after
https://crbug.com/646113 is finished, because that base::Value refactoring might
introduce some instances (happened to me recently).

BUG=729393
==========

[vabr (Chromium), 2017-06-04 10:56:16]

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

[vabr (Chromium), 2017-06-04 10:56:17]

Hi dcheng@,
Could you please review?

Hi jdoerrie@,
No need for you to review, just wanted to make you aware of this tool and of my
plan to run it after the base::Value refactoring is finished.

Cheers,
Vaclav

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
File tools/clang/move_raw/MoveRaw.cpp (right):

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
tools/clang/move_raw/MoveRaw.cpp:83: llvm::outs() << "==== BEGIN EDITS ====\n";
I was wondering if I should only emit BEGIN EDITS and END EDITS if there is >0
replacements, to avoid log spam. But the existing tools I saw always emit those,
even if no replacements are found.

[vabr (Chromium), 2017-06-04 11:01:53]

The CQ bit was checked by vabr@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-06-04 11:02:02]

Dry run: CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-04 11:13:39]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-06-04 11:13:40]

Dry run: This issue passed the CQ dry run.

[vabr (Chromium), 2017-06-04 13:22:24]

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
File tools/clang/move_raw/MoveRaw.cpp (right):

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
tools/clang/move_raw/MoveRaw.cpp:44: auto err =
replacements_->add(clang::tooling::Replacement(
It turned out that this hit errors in ~120 files where replacements from
multiple files were combined (a header and the .cc file, or two headers).
Looking at http://clang.llvm.org/docs/LibASTMatchersTutorial.html, checking
headers is actually not so useful and can be avoided by inserting, e.g., the
following block before line 44: 

  if (!callsite ||
      !result.SourceManager->isWrittenInMainFile(
          result.SourceManager->getSpellingLoc(callsite->getLocStart()))) {
    return;
  }

When I did that, I was able to eliminate all errors and find more instances of
std::move(raw) in .cc/.cpp files.

However, checking headers at least once does make sense to me, and I'm not sure
if that happens -- when I ran run_tool.py with a header file as an argument,
there was no output, even though the header itself contained std::move(raw).
Perhaps instead of checking isWrittenInMainFile I should check something else
which also works for headers?

[dcheng, 2017-06-07 19:58:34]

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
File tools/clang/move_raw/MoveRaw.cpp (right):

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
tools/clang/move_raw/MoveRaw.cpp:44: auto err =
replacements_->add(clang::tooling::Replacement(
On 2017/06/04 13:22:24, vabr (Chromium) wrote:
> It turned out that this hit errors in ~120 files where replacements from
> multiple files were combined (a header and the .cc file, or two headers).
> Looking at http://clang.llvm.org/docs/LibASTMatchersTutorial.html, checking
> headers is actually not so useful and can be avoided by inserting, e.g., the
> following block before line 44: 
> 
>   if (!callsite ||
>       !result.SourceManager->isWrittenInMainFile(
>           result.SourceManager->getSpellingLoc(callsite->getLocStart()))) {
>     return;
>   }
> 
> When I did that, I was able to eliminate all errors and find more instances of
> std::move(raw) in .cc/.cpp files.
> 
> However, checking headers at least once does make sense to me, and I'm not
sure
> if that happens -- when I ran run_tool.py with a header file as an argument,
> there was no output, even though the header itself contained std::move(raw).
> Perhaps instead of checking isWrittenInMainFile I should check something else
> which also works for headers?

I would suggest just using a std::set<clang::tooling::Replacement> instead of
the built-in Replacements class.

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
tools/clang/move_raw/MoveRaw.cpp:83: llvm::outs() << "==== BEGIN EDITS ====\n";
On 2017/06/04 10:56:17, vabr (Chromium) wrote:
> I was wondering if I should only emit BEGIN EDITS and END EDITS if there is >0
> replacements, to avoid log spam. But the existing tools I saw always emit
those,
> even if no replacements are found.

I don't think it matters too much, since we need to postprocess these through a
script anyway.

[vabr (Chromium), 2017-06-08 12:37:49]

Thanks for the review!

Please take another look.

Cheers,
Vaclav

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
File tools/clang/move_raw/MoveRaw.cpp (right):

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
tools/clang/move_raw/MoveRaw.cpp:44: auto err =
replacements_->add(clang::tooling::Replacement(
On 2017/06/07 19:58:34, dcheng wrote:
> On 2017/06/04 13:22:24, vabr (Chromium) wrote:
> > It turned out that this hit errors in ~120 files where replacements from
> > multiple files were combined (a header and the .cc file, or two headers).
> > Looking at http://clang.llvm.org/docs/LibASTMatchersTutorial.html, checking
> > headers is actually not so useful and can be avoided by inserting, e.g., the
> > following block before line 44: 
> > 
> >   if (!callsite ||
> >       !result.SourceManager->isWrittenInMainFile(
> >           result.SourceManager->getSpellingLoc(callsite->getLocStart()))) {
> >     return;
> >   }
> > 
> > When I did that, I was able to eliminate all errors and find more instances
of
> > std::move(raw) in .cc/.cpp files.
> > 
> > However, checking headers at least once does make sense to me, and I'm not
> sure
> > if that happens -- when I ran run_tool.py with a header file as an argument,
> > there was no output, even though the header itself contained std::move(raw).
> > Perhaps instead of checking isWrittenInMainFile I should check something
else
> > which also works for headers?
> 
> I would suggest just using a std::set<clang::tooling::Replacement> instead of
> the built-in Replacements class.

Done, with vector, because I'm not sure I need the uniqueness or sorting of set.

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
tools/clang/move_raw/MoveRaw.cpp:83: llvm::outs() << "==== BEGIN EDITS ====\n";
On 2017/06/07 19:58:34, dcheng wrote:
> On 2017/06/04 10:56:17, vabr (Chromium) wrote:
> > I was wondering if I should only emit BEGIN EDITS and END EDITS if there is
>0
> > replacements, to avoid log spam. But the existing tools I saw always emit
> those,
> > even if no replacements are found.
> 
> I don't think it matters too much, since we need to postprocess these through
a
> script anyway.

If it does not matter much, then I propose to make it silent for cases with no
replacements. Dumping tens of thousands of unnecessary lines and then filtering
them again makes processing slow, even if it is done by scripts.

[dcheng, 2017-06-08 19:40:43]

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
File tools/clang/move_raw/MoveRaw.cpp (right):

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
tools/clang/move_raw/MoveRaw.cpp:44: auto err =
replacements_->add(clang::tooling::Replacement(
On 2017/06/08 12:37:49, vabr (Chromium) wrote:
> On 2017/06/07 19:58:34, dcheng wrote:
> > On 2017/06/04 13:22:24, vabr (Chromium) wrote:
> > > It turned out that this hit errors in ~120 files where replacements from
> > > multiple files were combined (a header and the .cc file, or two headers).
> > > Looking at http://clang.llvm.org/docs/LibASTMatchersTutorial.html,
checking
> > > headers is actually not so useful and can be avoided by inserting, e.g.,
the
> > > following block before line 44: 
> > > 
> > >   if (!callsite ||
> > >       !result.SourceManager->isWrittenInMainFile(
> > >           result.SourceManager->getSpellingLoc(callsite->getLocStart())))
{
> > >     return;
> > >   }
> > > 
> > > When I did that, I was able to eliminate all errors and find more
instances
> of
> > > std::move(raw) in .cc/.cpp files.
> > > 
> > > However, checking headers at least once does make sense to me, and I'm not
> > sure
> > > if that happens -- when I ran run_tool.py with a header file as an
argument,
> > > there was no output, even though the header itself contained
std::move(raw).
> > > Perhaps instead of checking isWrittenInMainFile I should check something
> else
> > > which also works for headers?
> > 
> > I would suggest just using a std::set<clang::tooling::Replacement> instead
of
> > the built-in Replacements class.
> 
> Done, with vector, because I'm not sure I need the uniqueness or sorting of
set.

Well, why emit duplicates? =)

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
tools/clang/move_raw/MoveRaw.cpp:83: llvm::outs() << "==== BEGIN EDITS ====\n";
On 2017/06/08 12:37:49, vabr (Chromium) wrote:
> On 2017/06/07 19:58:34, dcheng wrote:
> > On 2017/06/04 10:56:17, vabr (Chromium) wrote:
> > > I was wondering if I should only emit BEGIN EDITS and END EDITS if there
is
> >0
> > > replacements, to avoid log spam. But the existing tools I saw always emit
> > those,
> > > even if no replacements are found.
> > 
> > I don't think it matters too much, since we need to postprocess these
through
> a
> > script anyway.
> 
> If it does not matter much, then I propose to make it silent for cases with no
> replacements. Dumping tens of thousands of unnecessary lines and then
filtering
> them again makes processing slow, even if it is done by scripts.

*shrug*

I don't feel strongly. But 2 extra lines in several gigabytes of output is
unlikely to make a real difference.

[vabr (Chromium), 2017-06-09 07:02:58]

Hi dcheng!

I changed vector to set, but perhaps we have a little misunderstanding about the
BEGIN/END in logs?

Thanks,
Vaclav

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
File tools/clang/move_raw/MoveRaw.cpp (right):

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
tools/clang/move_raw/MoveRaw.cpp:44: auto err =
replacements_->add(clang::tooling::Replacement(
On 2017/06/08 19:40:42, dcheng wrote:
> On 2017/06/08 12:37:49, vabr (Chromium) wrote:
> > On 2017/06/07 19:58:34, dcheng wrote:
> > > On 2017/06/04 13:22:24, vabr (Chromium) wrote:
> > > > It turned out that this hit errors in ~120 files where replacements from
> > > > multiple files were combined (a header and the .cc file, or two
headers).
> > > > Looking at http://clang.llvm.org/docs/LibASTMatchersTutorial.html,
> checking
> > > > headers is actually not so useful and can be avoided by inserting, e.g.,
> the
> > > > following block before line 44: 
> > > > 
> > > >   if (!callsite ||
> > > >       !result.SourceManager->isWrittenInMainFile(
> > > >          
result.SourceManager->getSpellingLoc(callsite->getLocStart())))
> {
> > > >     return;
> > > >   }
> > > > 
> > > > When I did that, I was able to eliminate all errors and find more
> instances
> > of
> > > > std::move(raw) in .cc/.cpp files.
> > > > 
> > > > However, checking headers at least once does make sense to me, and I'm
not
> > > sure
> > > > if that happens -- when I ran run_tool.py with a header file as an
> argument,
> > > > there was no output, even though the header itself contained
> std::move(raw).
> > > > Perhaps instead of checking isWrittenInMainFile I should check something
> > else
> > > > which also works for headers?
> > > 
> > > I would suggest just using a std::set<clang::tooling::Replacement> instead
> of
> > > the built-in Replacements class.
> > 
> > Done, with vector, because I'm not sure I need the uniqueness or sorting of
> set.
> 
> Well, why emit duplicates? =)

By "not sure I need the uniqueness" I meant "I cannot imagine how there could be
duplicates". :) But this container has on the order of units of elements, so I
am not concerned with unnecessary overhead coming from set, and I will change it
to set.

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
tools/clang/move_raw/MoveRaw.cpp:83: llvm::outs() << "==== BEGIN EDITS ====\n";
On 2017/06/08 19:40:42, dcheng wrote:
> On 2017/06/08 12:37:49, vabr (Chromium) wrote:
> > On 2017/06/07 19:58:34, dcheng wrote:
> > > On 2017/06/04 10:56:17, vabr (Chromium) wrote:
> > > > I was wondering if I should only emit BEGIN EDITS and END EDITS if there
> is
> > >0
> > > > replacements, to avoid log spam. But the existing tools I saw always
emit
> > > those,
> > > > even if no replacements are found.
> > > 
> > > I don't think it matters too much, since we need to postprocess these
> through
> > a
> > > script anyway.
> > 
> > If it does not matter much, then I propose to make it silent for cases with
no
> > replacements. Dumping tens of thousands of unnecessary lines and then
> filtering
> > them again makes processing slow, even if it is done by scripts.
> 
> *shrug*
> 
> I don't feel strongly. But 2 extra lines in several gigabytes of output is
> unlikely to make a real difference.

It was not 2 extra lines. When I ran the tool, tens thousands of files
contributed just the empty BEGIN/END sequence, with about 10-20 contributing
also some replacements. The file had close to 100000 lines, and only about 100
of them were actually useful.

[dcheng, 2017-06-09 07:06:41]

We probably want to eventually turn this into a clang-tidy check, maybe add a
TODO to that effect somewhere.

Other than that, LGTM

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
File tools/clang/move_raw/MoveRaw.cpp (right):

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
tools/clang/move_raw/MoveRaw.cpp:83: llvm::outs() << "==== BEGIN EDITS ====\n";
On 2017/06/09 07:02:57, vabr (Chromium) wrote:
> On 2017/06/08 19:40:42, dcheng wrote:
> > On 2017/06/08 12:37:49, vabr (Chromium) wrote:
> > > On 2017/06/07 19:58:34, dcheng wrote:
> > > > On 2017/06/04 10:56:17, vabr (Chromium) wrote:
> > > > > I was wondering if I should only emit BEGIN EDITS and END EDITS if
there
> > is
> > > >0
> > > > > replacements, to avoid log spam. But the existing tools I saw always
> emit
> > > > those,
> > > > > even if no replacements are found.
> > > > 
> > > > I don't think it matters too much, since we need to postprocess these
> > through
> > > a
> > > > script anyway.
> > > 
> > > If it does not matter much, then I propose to make it silent for cases
with
> no
> > > replacements. Dumping tens of thousands of unnecessary lines and then
> > filtering
> > > them again makes processing slow, even if it is done by scripts.
> > 
> > *shrug*
> > 
> > I don't feel strongly. But 2 extra lines in several gigabytes of output is
> > unlikely to make a real difference.
> 
> It was not 2 extra lines. When I ran the tool, tens thousands of files
> contributed just the empty BEGIN/END sequence, with about 10-20 contributing
> also some replacements. The file had close to 100000 lines, and only about 100
> of them were actually useful.

Sorry, to clarify, I mean 2 extra lines per invocation of the tool. I'm
surprised that there were 100k lines of output though: IIRC, there's only
something like 25k .cc targets, so I would have expected something on the order
of ~50k lines maybe.

I don't feel super strongly about it.

[dcheng, 2017-06-09 07:08:33]

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
File tools/clang/move_raw/MoveRaw.cpp (right):

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
tools/clang/move_raw/MoveRaw.cpp:44: auto err =
replacements_->add(clang::tooling::Replacement(
On 2017/06/09 07:02:57, vabr (Chromium) wrote:
> On 2017/06/08 19:40:42, dcheng wrote:
> > On 2017/06/08 12:37:49, vabr (Chromium) wrote:
> > > On 2017/06/07 19:58:34, dcheng wrote:
> > > > On 2017/06/04 13:22:24, vabr (Chromium) wrote:
> > > > > It turned out that this hit errors in ~120 files where replacements
from
> > > > > multiple files were combined (a header and the .cc file, or two
> headers).
> > > > > Looking at http://clang.llvm.org/docs/LibASTMatchersTutorial.html,
> > checking
> > > > > headers is actually not so useful and can be avoided by inserting,
e.g.,
> > the
> > > > > following block before line 44: 
> > > > > 
> > > > >   if (!callsite ||
> > > > >       !result.SourceManager->isWrittenInMainFile(
> > > > >          
> result.SourceManager->getSpellingLoc(callsite->getLocStart())))
> > {
> > > > >     return;
> > > > >   }
> > > > > 
> > > > > When I did that, I was able to eliminate all errors and find more
> > instances
> > > of
> > > > > std::move(raw) in .cc/.cpp files.
> > > > > 
> > > > > However, checking headers at least once does make sense to me, and I'm
> not
> > > > sure
> > > > > if that happens -- when I ran run_tool.py with a header file as an
> > argument,
> > > > > there was no output, even though the header itself contained
> > std::move(raw).
> > > > > Perhaps instead of checking isWrittenInMainFile I should check
something
> > > else
> > > > > which also works for headers?
> > > > 
> > > > I would suggest just using a std::set<clang::tooling::Replacement>
instead
> > of
> > > > the built-in Replacements class.
> > > 
> > > Done, with vector, because I'm not sure I need the uniqueness or sorting
of
> > set.
> > 
> > Well, why emit duplicates? =)
> 
> By "not sure I need the uniqueness" I meant "I cannot imagine how there could
be
> duplicates". :) But this container has on the order of units of elements, so I
> am not concerned with unnecessary overhead coming from set, and I will change
it
> to set.

I suppose in the context of how these tools are typically run (via the
run_tool.py helper), it won't happen. Generally speaking though, it's possible
to run one invocation of the clang tool across multiple files, and you can end
up with duplicates in that case.

[vabr (Chromium), 2017-06-09 07:12:36]

Description was changed from

==========
Add a clang tool to detect std::move(raw ptr)

The CL also adds a sentence to the documentation for clang tools, to supply a
bit which was unclear for me during writing of the added tool.

So far, on Linux, the tool found instances of std::move(raw ptr) in:

../../components/browser_sync/profile_sync_service.cc:10101
../../components/sync_sessions/sessions_sync_manager.cc:5987
../../content/browser/indexed_db/indexed_db_transaction.cc:3865
../../gpu/ipc/service/gpu_channel.cc:26467
../../services/preferences/tracked/pref_hash_store_impl.cc:3329
../../services/preferences/tracked/pref_hash_store_impl.cc:4518
../../services/ui/public/cpp/bitmap/child_shared_bitmap_manager.cc:5009
../../services/video_capture/test/mock_device_factory.cc:2266

It also found it in some 3rd party headers, such as
../../third_party/flatbuffers/src/include/flatbuffers/flatbuffers.h:8260
../../third_party/skia/include/core/SkCanvas.h:14756
../../third_party/webrtc/api/proxy.h:16937
../../third_party/webrtc/api/proxy.h:19681
../../third_party/webrtc/api/proxy.h:20241
and c++ libs.

The Chromium 1st party instances will be addressed in separate CLs. For the
headers, I'll consider if it makes sense to address this (e.g., some occurrences
seem to be template instantiations, where the template is not a priori aware
that the supplied type is a raw pointer; in such cases std::move should stay).

I may also run the tool on more platforms.

Finally, while preventing regressions would be good, it seems impractical to run
this tool as a presubmit check. However, I will try to run it after
https://crbug.com/646113 is finished, because that base::Value refactoring might
introduce some instances (happened to me recently).

BUG=729393
==========

to

==========
Add a clang tool to detect std::move(raw ptr)

The CL also adds a sentence to the documentation for clang tools, to supply a
bit which was unclear for me during writing of the added tool.

So far, on Linux, the tool found instances of std::move(raw ptr) in:

../../components/browser_sync/profile_sync_service.cc:10101
../../components/sync_sessions/sessions_sync_manager.cc:5987
../../content/browser/indexed_db/indexed_db_transaction.cc:3865
../../gpu/ipc/service/gpu_channel.cc:26467
../../services/preferences/tracked/pref_hash_store_impl.cc:3329
../../services/preferences/tracked/pref_hash_store_impl.cc:4518
../../services/ui/public/cpp/bitmap/child_shared_bitmap_manager.cc:5009
../../services/video_capture/test/mock_device_factory.cc:2266

It also found it in some 3rd party headers, such as
../../third_party/flatbuffers/src/include/flatbuffers/flatbuffers.h:8260
../../third_party/skia/include/core/SkCanvas.h:14756
../../third_party/webrtc/api/proxy.h:16937
../../third_party/webrtc/api/proxy.h:19681
../../third_party/webrtc/api/proxy.h:20241
and c++ libs.

The Chromium 1st party instances will be addressed in separate CLs. For the
headers, I'll consider if it makes sense to address this (e.g., some occurrences
seem to be template instantiations, where the template is not a priori aware
that the supplied type is a raw pointer; in such cases std::move should stay).

I may also run the tool on more platforms.

Finally, while preventing regressions would be good, it seems impractical to run
this tool as a presubmit check. However, I will try to run it after
https://crbug.com/646113 is finished, because that base::Value refactoring might
introduce some instances (happened to me recently).

BUG=729393,731577
==========

[vabr (Chromium), 2017-06-09 07:19:38]

Thank you for the quick response!

I added the TODO and https://crbug.com/731577 to track the conversion to
clang-tidy, and will be landing this now.

Cheers,
Vaclav

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
File tools/clang/move_raw/MoveRaw.cpp (right):

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
tools/clang/move_raw/MoveRaw.cpp:44: auto err =
replacements_->add(clang::tooling::Replacement(
On 2017/06/09 07:08:32, dcheng wrote:
> On 2017/06/09 07:02:57, vabr (Chromium) wrote:
> > On 2017/06/08 19:40:42, dcheng wrote:
> > > On 2017/06/08 12:37:49, vabr (Chromium) wrote:
> > > > On 2017/06/07 19:58:34, dcheng wrote:
> > > > > On 2017/06/04 13:22:24, vabr (Chromium) wrote:
> > > > > > It turned out that this hit errors in ~120 files where replacements
> from
> > > > > > multiple files were combined (a header and the .cc file, or two
> > headers).
> > > > > > Looking at http://clang.llvm.org/docs/LibASTMatchersTutorial.html,
> > > checking
> > > > > > headers is actually not so useful and can be avoided by inserting,
> e.g.,
> > > the
> > > > > > following block before line 44: 
> > > > > > 
> > > > > >   if (!callsite ||
> > > > > >       !result.SourceManager->isWrittenInMainFile(
> > > > > >          
> > result.SourceManager->getSpellingLoc(callsite->getLocStart())))
> > > {
> > > > > >     return;
> > > > > >   }
> > > > > > 
> > > > > > When I did that, I was able to eliminate all errors and find more
> > > instances
> > > > of
> > > > > > std::move(raw) in .cc/.cpp files.
> > > > > > 
> > > > > > However, checking headers at least once does make sense to me, and
I'm
> > not
> > > > > sure
> > > > > > if that happens -- when I ran run_tool.py with a header file as an
> > > argument,
> > > > > > there was no output, even though the header itself contained
> > > std::move(raw).
> > > > > > Perhaps instead of checking isWrittenInMainFile I should check
> something
> > > > else
> > > > > > which also works for headers?
> > > > > 
> > > > > I would suggest just using a std::set<clang::tooling::Replacement>
> instead
> > > of
> > > > > the built-in Replacements class.
> > > > 
> > > > Done, with vector, because I'm not sure I need the uniqueness or sorting
> of
> > > set.
> > > 
> > > Well, why emit duplicates? =)
> > 
> > By "not sure I need the uniqueness" I meant "I cannot imagine how there
could
> be
> > duplicates". :) But this container has on the order of units of elements, so
I
> > am not concerned with unnecessary overhead coming from set, and I will
change
> it
> > to set.
> 
> I suppose in the context of how these tools are typically run (via the
> run_tool.py helper), it won't happen. Generally speaking though, it's possible
> to run one invocation of the clang tool across multiple files, and you can end
> up with duplicates in that case.

Got it, thanks! I agree that set is a better choice here for this reason.

https://codereview.chromium.org/2919243002/diff/1/tools/clang/move_raw/MoveRa...
tools/clang/move_raw/MoveRaw.cpp:83: llvm::outs() << "==== BEGIN EDITS ====\n";
On 2017/06/09 07:06:41, dcheng wrote:
> On 2017/06/09 07:02:57, vabr (Chromium) wrote:
> > On 2017/06/08 19:40:42, dcheng wrote:
> > > On 2017/06/08 12:37:49, vabr (Chromium) wrote:
> > > > On 2017/06/07 19:58:34, dcheng wrote:
> > > > > On 2017/06/04 10:56:17, vabr (Chromium) wrote:
> > > > > > I was wondering if I should only emit BEGIN EDITS and END EDITS if
> there
> > > is
> > > > >0
> > > > > > replacements, to avoid log spam. But the existing tools I saw always
> > emit
> > > > > those,
> > > > > > even if no replacements are found.
> > > > > 
> > > > > I don't think it matters too much, since we need to postprocess these
> > > through
> > > > a
> > > > > script anyway.
> > > > 
> > > > If it does not matter much, then I propose to make it silent for cases
> with
> > no
> > > > replacements. Dumping tens of thousands of unnecessary lines and then
> > > filtering
> > > > them again makes processing slow, even if it is done by scripts.
> > > 
> > > *shrug*
> > > 
> > > I don't feel strongly. But 2 extra lines in several gigabytes of output is
> > > unlikely to make a real difference.
> > 
> > It was not 2 extra lines. When I ran the tool, tens thousands of files
> > contributed just the empty BEGIN/END sequence, with about 10-20 contributing
> > also some replacements. The file had close to 100000 lines, and only about
100
> > of them were actually useful.
> 
> Sorry, to clarify, I mean 2 extra lines per invocation of the tool. I'm
> surprised that there were 100k lines of output though: IIRC, there's only
> something like 25k .cc targets, so I would have expected something on the
order
> of ~50k lines maybe.
> 
> I don't feel super strongly about it.

My apologies, I was sloppy with the estimate (I did not actually count the
lines, because that file is long gone by now). ~50k sounds more plausible for
what it had. 

IMO, even with that update the empty BEGIN/END blocks make a vast majority of
the large result, so given you don't feel strongly, I will keep the early exit
without logging if there are new replacements (introduced in patch set 3).

[vabr (Chromium), 2017-06-09 07:19:44]

The CQ bit was checked by vabr@chromium.org

[vabr (Chromium), 2017-06-09 07:19:44]

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/2919243002/#ps80001
(title: "Add a TODO to make this a clang-tidy check")

[commit-bot: I haz the power, 2017-06-09 07:20:00]

CQ is trying da patch.

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

[commit-bot: I haz the power, 2017-06-09 07:30:45]

CQ is committing da patch.

Bot data: {"patchset_id": 80001, "attempt_start_ts": 1496992784244340,
"parent_rev": "fd5c855b027d953b3a995041440bed83b9313197", "commit_rev":
"9ed3f43a6cc49b844d769498283b2e1e0be6cb94"}

[commit-bot: I haz the power, 2017-06-09 07:30:58]

Description was changed from

==========
Add a clang tool to detect std::move(raw ptr)

The CL also adds a sentence to the documentation for clang tools, to supply a
bit which was unclear for me during writing of the added tool.

So far, on Linux, the tool found instances of std::move(raw ptr) in:

../../components/browser_sync/profile_sync_service.cc:10101
../../components/sync_sessions/sessions_sync_manager.cc:5987
../../content/browser/indexed_db/indexed_db_transaction.cc:3865
../../gpu/ipc/service/gpu_channel.cc:26467
../../services/preferences/tracked/pref_hash_store_impl.cc:3329
../../services/preferences/tracked/pref_hash_store_impl.cc:4518
../../services/ui/public/cpp/bitmap/child_shared_bitmap_manager.cc:5009
../../services/video_capture/test/mock_device_factory.cc:2266

It also found it in some 3rd party headers, such as
../../third_party/flatbuffers/src/include/flatbuffers/flatbuffers.h:8260
../../third_party/skia/include/core/SkCanvas.h:14756
../../third_party/webrtc/api/proxy.h:16937
../../third_party/webrtc/api/proxy.h:19681
../../third_party/webrtc/api/proxy.h:20241
and c++ libs.

The Chromium 1st party instances will be addressed in separate CLs. For the
headers, I'll consider if it makes sense to address this (e.g., some occurrences
seem to be template instantiations, where the template is not a priori aware
that the supplied type is a raw pointer; in such cases std::move should stay).

I may also run the tool on more platforms.

Finally, while preventing regressions would be good, it seems impractical to run
this tool as a presubmit check. However, I will try to run it after
https://crbug.com/646113 is finished, because that base::Value refactoring might
introduce some instances (happened to me recently).

BUG=729393,731577
==========

to

==========
Add a clang tool to detect std::move(raw ptr)

The CL also adds a sentence to the documentation for clang tools, to supply a
bit which was unclear for me during writing of the added tool.

So far, on Linux, the tool found instances of std::move(raw ptr) in:

../../components/browser_sync/profile_sync_service.cc:10101
../../components/sync_sessions/sessions_sync_manager.cc:5987
../../content/browser/indexed_db/indexed_db_transaction.cc:3865
../../gpu/ipc/service/gpu_channel.cc:26467
../../services/preferences/tracked/pref_hash_store_impl.cc:3329
../../services/preferences/tracked/pref_hash_store_impl.cc:4518
../../services/ui/public/cpp/bitmap/child_shared_bitmap_manager.cc:5009
../../services/video_capture/test/mock_device_factory.cc:2266

It also found it in some 3rd party headers, such as
../../third_party/flatbuffers/src/include/flatbuffers/flatbuffers.h:8260
../../third_party/skia/include/core/SkCanvas.h:14756
../../third_party/webrtc/api/proxy.h:16937
../../third_party/webrtc/api/proxy.h:19681
../../third_party/webrtc/api/proxy.h:20241
and c++ libs.

The Chromium 1st party instances will be addressed in separate CLs. For the
headers, I'll consider if it makes sense to address this (e.g., some occurrences
seem to be template instantiations, where the template is not a priori aware
that the supplied type is a raw pointer; in such cases std::move should stay).

I may also run the tool on more platforms.

Finally, while preventing regressions would be good, it seems impractical to run
this tool as a presubmit check. However, I will try to run it after
https://crbug.com/646113 is finished, because that base::Value refactoring might
introduce some instances (happened to me recently).

BUG=729393,731577

Review-Url: https://codereview.chromium.org/2919243002
Cr-Commit-Position: refs/heads/master@{#478226}
Committed:
https://chromium.googlesource.com/chromium/src/+/9ed3f43a6cc49b844d769498283b...
==========

[commit-bot: I haz the power, 2017-06-09 07:30:59]

Committed patchset #5 (id:80001) as
https://chromium.googlesource.com/chromium/src/+/9ed3f43a6cc49b844d769498283b...

[Nico, 2017-06-12 10:48:29]

On 2017/06/09 07:30:59, commit-bot: I haz the power wrote:
> Committed patchset #5 (id:80001) as
>
https://chromium.googlesource.com/chromium/src/+/9ed3f43a6cc49b844d769498283b...

Wouldn't this make more sense as a compiler warning? Why is this a tool?

[vabr (Chromium), 2017-06-12 10:58:04]

On 2017/06/12 10:48:29, Nico (vacation Jun 3-11) wrote:

> Wouldn't this make more sense as a compiler warning?

I would not be opposed to it. I just don't know what is the bar for a check to
make it into compiler warnings. dcheng@ proposed to make this a clang-tidy check
(https://crbug.com/731577).

> Why is this a tool?

I would like to run this after relevant refactorings (such as
https://crbug.com/646113). Adding it to the tools seemed like the most
straightforward idea to me, but I'm open to changing that.

Cheers,
Vaclav

[vabr (Chromium), 2017-06-12 13:07:49]

So today I found out that a colleague (dvadym@) implemented this as a clang-tidy
check some while ago.
So much for coding during holiday and not consulting people sitting around. :)

I'm happy to remove this tool, given that it is covered by clang-tidy already.
Please let me know (dcheng@ or thakis@) if you would like me to do that.

Cheers,
Vaclav

[Nico, 2017-06-13 14:05:27]

On Mon, Jun 12, 2017 at 6:48 AM, <thakis@chromium.org> wrote:

> On 2017/06/09 07:30:59, commit-bot: I haz the power wrote:
> > Committed patchset #5 (id:80001) as
> >
> https://chromium.googlesource.com/chromium/src/+/
> 9ed3f43a6cc49b844d769498283b2e1e0be6cb94
>
> Wouldn't this make more sense as a compiler warning? Why is this a tool?
>

vabr, dcheng: ping ^


>
> https://codereview.chromium.org/2919243002/
>

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.

[Nico, 2017-06-13 16:11:45]

Sorry, hadn't seen your reply.

I think this probably passes the bar for a compiler warning, and with a fix it
on the diag it could clean up code as well. So I think that'd be even better
than having this in clang-tidy.

If this already exists in clang-tidy, we should probably remove this again --
what do you think?

[vabr (Chromium), 2017-06-13 17:25:53]

Thanks, Nico, for the answer.

As for clang-tidy, as long as using it is not substantially harder than running
the tool, I'm all for removing this tool. Reading
https://chromium.googlesource.com/chromium/src/+/lkcr/docs/clang_tidy.md, it is
reported "experimental" and "somewhat painful to use". Is this comparable to
clang tools? In particular, I loved how clang tools got the llvm+clang repos and
bootstrapped clang with one comment. That should presumably also work for
clang-tidy?

As for the warning, that sounds also good, although there the TODO is to avoid
templated code (currently source of false-positives in this tool). I sadly never
contributed to clang before, but I have Google's clang team in a walking
distance, so hopefully this could work. I'm not sure when I could get to it, but
it does not sound urgent.

Cheers,
Vaclav

[dcheng, 2017-06-13 17:30:11]

On 2017/06/13 17:25:53, vabr (Chromium) wrote:
> Thanks, Nico, for the answer.
> 
> As for clang-tidy, as long as using it is not substantially harder than
running
> the tool, I'm all for removing this tool. Reading
> https://chromium.googlesource.com/chromium/src/+/lkcr/docs/clang_tidy.md, it
is
> reported "experimental" and "somewhat painful to use". Is this comparable to
> clang tools? In particular, I loved how clang tools got the llvm+clang repos
and
> bootstrapped clang with one comment. That should presumably also work for
> clang-tidy?
> 
> As for the warning, that sounds also good, although there the TODO is to avoid
> templated code (currently source of false-positives in this tool). I sadly
never
> contributed to clang before, but I have Google's clang team in a walking
> distance, so hopefully this could work. I'm not sure when I could get to it,
but
> it does not sound urgent.
> 
> Cheers,
> Vaclav

It was painful the last time I tried it, but I filed a few bugs and there were
some fixes. So it's possible the situation is better now. It's worth a shot
anyway.

[vabr (Chromium), 2017-06-13 17:40:08]

On 2017/06/13 17:30:11, dcheng wrote:
> On 2017/06/13 17:25:53, vabr (Chromium) wrote:
> > Thanks, Nico, for the answer.
> > 
> > As for clang-tidy, as long as using it is not substantially harder than
> running
> > the tool, I'm all for removing this tool. Reading
> > https://chromium.googlesource.com/chromium/src/+/lkcr/docs/clang_tidy.md, it
> is
> > reported "experimental" and "somewhat painful to use". Is this comparable to
> > clang tools? In particular, I loved how clang tools got the llvm+clang repos
> and
> > bootstrapped clang with one comment. That should presumably also work for
> > clang-tidy?
> > 
> > As for the warning, that sounds also good, although there the TODO is to
avoid
> > templated code (currently source of false-positives in this tool). I sadly
> never
> > contributed to clang before, but I have Google's clang team in a walking
> > distance, so hopefully this could work. I'm not sure when I could get to it,
> but
> > it does not sound urgent.
> > 
> > Cheers,
> > Vaclav
> 
> It was painful the last time I tried it, but I filed a few bugs and there were
> some fixes. So it's possible the situation is better now. It's worth a shot
> anyway.

OK, so my plan for clang-tidy would be to try it out, and update the above
linked doc in case improvements (like the automated checking of llvm+clang) are
ready. If clang-tidy does a reasonable job for checking the std::move(raw) case,
I will submit a CL deleting the tool. If not, I will bring up the issues with
both of you to check what would be the best next steps.

Unless you feel this is urgent, it might take me possibly a low number of weeks.
Track the progress at https://crbug.com/732867.

Cheers,
Vaclav

[Nico, 2017-06-13 17:42:01]

On Tue, Jun 13, 2017 at 1:40 PM, <vabr@chromium.org> wrote:

> On 2017/06/13 17:30:11, dcheng wrote:
> > On 2017/06/13 17:25:53, vabr (Chromium) wrote:
> > > Thanks, Nico, for the answer.
> > >
> > > As for clang-tidy, as long as using it is not substantially harder than
> > running
> > > the tool, I'm all for removing this tool. Reading
> > > https://chromium.googlesource.com/chromium/src/+/lkcr/docs/
> clang_tidy.md, it
> > is
> > > reported "experimental" and "somewhat painful to use". Is this
> comparable to
> > > clang tools? In particular, I loved how clang tools got the llvm+clang
> repos
> > and
> > > bootstrapped clang with one comment. That should presumably also work
> for
> > > clang-tidy?
> > >
> > > As for the warning, that sounds also good, although there the TODO is
> to
> avoid
> > > templated code (currently source of false-positives in this tool). I
> sadly
> > never
> > > contributed to clang before, but I have Google's clang team in a
> walking
> > > distance, so hopefully this could work. I'm not sure when I could get
> to it,
> > but
> > > it does not sound urgent.
> > >
> > > Cheers,
> > > Vaclav
> >
> > It was painful the last time I tried it, but I filed a few bugs and
> there were
> > some fixes. So it's possible the situation is better now. It's worth a
> shot
> > anyway.
>
> OK, so my plan for clang-tidy would be to try it out, and update the above
> linked doc in case improvements (like the automated checking of
> llvm+clang) are
> ready. If clang-tidy does a reasonable job for checking the std::move(raw)
> case,
> I will submit a CL deleting the tool. If not, I will bring up the issues
> with
> both of you to check what would be the best next steps.
>
> Unless you feel this is urgent, it might take me possibly a low number of
> weeks.
> Track the progress at https://crbug.com/732867.
>

Thanks, that sounds like a good plan.

-- 
You received this message because you are subscribed to the Google Groups
"Chromium-reviews" group.
To unsubscribe from this group and stop receiving emails from it, send an email
to chromium-reviews+unsubscribe@chromium.org.