New issue
Advanced search Search tips

Issue 777751 link

Starred by 1 user
Status: Fixed
Owner:
jbroman@chromium.org
Closed: Nov 20
Cc:
tzik@chromium.org
jbroman@chromium.org
dcheng@chromium.org
yutak@chromium.org
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 3
Type: Bug



Sign in to add a comment

GC plugin: Check that std::make_unique<>, base::optional<> etc are not used for GarbageCollected objects

Project Member Reported by haraken@chromium.org, Oct 24 2017 
We want to replace WTF::Optional with base::optional. We want to replace WTF::MakeUnique with std::make_unique.

To make that happen, we have to migrate the static_assert(!IsGarbageCollectedType) check from WTF to the GC plugin.

WTF::WrapArrayUnique and WTF::WrapUnique will need the same treatment.

Comment 1 by yutak@chromium.org, Oct 24 2017 

This isn't easy since you need to look at a function body. If I remember
correctly we don't have a check against statements in a function.

(FWIW I still think using WTF wrappers is appropriate and is a better approach.)

Comment 2 by tzik@chromium.org, Oct 25 2017 

Is it hard to add a visitor to statements?

Comment 3 by jbroman@chromium.org, Oct 25 2017 

I can probably take a quick crack at this. No, it shouldn't be hard to match statements.
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 26 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/9624ee2a5b59e1a5a6053e95ec98a01f76cf92d3

commit 9624ee2a5b59e1a5a6053e95ec98a01f76cf92d3
Author: Jeremy Roman <jbroman@chromium.org>
Date: Thu Oct 26 17:52:07 2017

Catch make_unique and WrapUnique applied to GC objects in the Blink GC plugin.

Implemented using Clang AST matchers for concise code.

Bug:  777751 
Change-Id: I37b5d14205314c5ada88e45f06b997006bdfda85
Reviewed-on: https://chromium-review.googlesource.com/738401
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Reviewed-by: Keishi Hattori <keishi@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511876}
[add] https://crrev.com/9624ee2a5b59e1a5a6053e95ec98a01f76cf92d3/tools/clang/blink_gc_plugin/BadPatternFinder.cpp
[add] https://crrev.com/9624ee2a5b59e1a5a6053e95ec98a01f76cf92d3/tools/clang/blink_gc_plugin/BadPatternFinder.h
[modify] https://crrev.com/9624ee2a5b59e1a5a6053e95ec98a01f76cf92d3/tools/clang/blink_gc_plugin/BlinkGCPluginConsumer.cpp
[modify] https://crrev.com/9624ee2a5b59e1a5a6053e95ec98a01f76cf92d3/tools/clang/blink_gc_plugin/CMakeLists.txt
[modify] https://crrev.com/9624ee2a5b59e1a5a6053e95ec98a01f76cf92d3/tools/clang/blink_gc_plugin/DiagnosticsReporter.cpp
[modify] https://crrev.com/9624ee2a5b59e1a5a6053e95ec98a01f76cf92d3/tools/clang/blink_gc_plugin/DiagnosticsReporter.h
[modify] https://crrev.com/9624ee2a5b59e1a5a6053e95ec98a01f76cf92d3/tools/clang/blink_gc_plugin/tests/heap/stubs.h
[add] https://crrev.com/9624ee2a5b59e1a5a6053e95ec98a01f76cf92d3/tools/clang/blink_gc_plugin/tests/make_unique_gc_object.cpp
[add] https://crrev.com/9624ee2a5b59e1a5a6053e95ec98a01f76cf92d3/tools/clang/blink_gc_plugin/tests/make_unique_gc_object.h
[add] https://crrev.com/9624ee2a5b59e1a5a6053e95ec98a01f76cf92d3/tools/clang/blink_gc_plugin/tests/make_unique_gc_object.txt
[modify] https://crrev.com/9624ee2a5b59e1a5a6053e95ec98a01f76cf92d3/tools/clang/blink_gc_plugin/tests/stack_allocated.txt
Project Member

Comment 5 by bugdroid1@chromium.org, Oct 26 2017 

The following revision refers to this bug:
  https://chromium.googlesource.com/chromium/src.git/+/d1d887e89d592d27741239ada9ec2833e465c366

commit d1d887e89d592d27741239ada9ec2833e465c366
Author: Jeremy Roman <jbroman@chromium.org>
Date: Thu Oct 26 19:08:04 2017

Catch base::Optional<T> for GC objects in the Blink GC plugin.

Implemented using Clang AST matchers for concise code.

Bug:  777751 
Change-Id: Id77478f3a785e36dba1c6bd340d03c763d9734e9
Reviewed-on: https://chromium-review.googlesource.com/739781
Commit-Queue: Jeremy Roman <jbroman@chromium.org>
Reviewed-by: Kentaro Hara <haraken@chromium.org>
Cr-Commit-Position: refs/heads/master@{#511910}
[modify] https://crrev.com/d1d887e89d592d27741239ada9ec2833e465c366/tools/clang/blink_gc_plugin/BadPatternFinder.cpp
[modify] https://crrev.com/d1d887e89d592d27741239ada9ec2833e465c366/tools/clang/blink_gc_plugin/DiagnosticsReporter.cpp
[modify] https://crrev.com/d1d887e89d592d27741239ada9ec2833e465c366/tools/clang/blink_gc_plugin/DiagnosticsReporter.h
[modify] https://crrev.com/d1d887e89d592d27741239ada9ec2833e465c366/tools/clang/blink_gc_plugin/tests/heap/stubs.h
[add] https://crrev.com/d1d887e89d592d27741239ada9ec2833e465c366/tools/clang/blink_gc_plugin/tests/optional_gc_object.cpp
[add] https://crrev.com/d1d887e89d592d27741239ada9ec2833e465c366/tools/clang/blink_gc_plugin/tests/optional_gc_object.h
[add] https://crrev.com/d1d887e89d592d27741239ada9ec2833e465c366/tools/clang/blink_gc_plugin/tests/optional_gc_object.txt
[modify] https://crrev.com/d1d887e89d592d27741239ada9ec2833e465c366/tools/clang/blink_gc_plugin/tests/stack_allocated.txt

Comment 6 by benhenry@chromium.org, Aug 1

Status: Assigned (was: Untriaged)

Comment 7 by jbroman@chromium.org, Nov 20

Owner: jbroman@chromium.org
Status: Fixed (was: Assigned)
...I think this is done.
►

Sign in to add a comment