clang plugin gets confused when inserting an override specifier for a defaulted function |
||
Issue description
diff --git a/third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h b/third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h
index f96565d805cb..2e16aa27c84d 100644
--- a/third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h
+++ b/third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h
@@ -31,10 +31,10 @@ class CORE_EXPORT ScriptedIdleTaskController
static ScriptedIdleTaskController* Create(ExecutionContext* context) {
return new ScriptedIdleTaskController(context);
}
- ~ScriptedIdleTaskController();
+ ~ScriptedIdleTaskController() override;
- void Trace(blink::Visitor*);
- void TraceWrappers(const ScriptWrappableVisitor*) const;
+ void Trace(blink::Visitor*) override;
+ void TraceWrappers(const ScriptWrappableVisitor*) const override;
using CallbackId = int;
@@ -44,7 +44,7 @@ class CORE_EXPORT ScriptedIdleTaskController
public TraceWrapperBase {
public:
virtual void Trace(blink::Visitor* visitor) {}
- virtual void TraceWrappers(const ScriptWrappableVisitor* visitor) const {}
+ virtual void TraceWrappers(const ScriptWrappableVisitor* visitor) const override{}
virtual ~IdleTask() = default;
virtual void invoke(IdleDeadline*) = 0;
};
@@ -56,10 +56,10 @@ class CORE_EXPORT ScriptedIdleTaskController
static V8IdleTask* Create(V8IdleRequestCallback* callback) {
return new V8IdleTask(callback);
}
- ~V8IdleTask() = default;
+ ~V8IdleTask() override= defaul overridet;
void invoke(IdleDeadline*) override;
- void Trace(blink::Visitor*);
- void TraceWrappers(const ScriptWrappableVisitor*) const;
+ void Trace(blink::Visitor*) override;
+ void TraceWrappers(const ScriptWrappableVisitor*) const override;
private:
explicit V8IdleTask(V8IdleRequestCallback*);
This is pretty clearly wrong.
Interestingly enough, it seems like we usually generate the right insertion... except for one file:
$ grep 'third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59' fixit_output
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59:19-59:19}:" override"
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59:19-59:19}:" override"
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59:19-59:19}:" override"
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59:27-59:27}:" override"
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59:19-59:19}:" override"
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59:19-59:19}:" override"
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59:19-59:19}:" override"
It's unclear if this is specific to the jumbo build or not: in a jumbo build. Here's the current location where the plugin generates the bad replacement:
In file included from gen/third_party/WebKit/Source/core/dom/dom_jumbo_3.cc:6:
In file included from gen/third_party/WebKit/Source/core/dom/../../../../../../../../third_party/WebKit/Source/core/dom/MutationObserver.cpp:44:
In file included from ../../third_party/WebKit/Source/core/probe/CoreProbes.h:36:
In file included from ../../third_party/WebKit/Source/core/dom/Document.h:51:
../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h:34:32: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
~ScriptedIdleTaskController();
^
override
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{34:32-34:32}:" override"
../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h:36:30: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
void Trace(blink::Visitor*);
^
override
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{36:30-36:30}:" override"
../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h:37:58: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
void TraceWrappers(const ScriptWrappableVisitor*) const;
^
override
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{37:58-37:58}:" override"
../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h:47:77: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
virtual void TraceWrappers(const ScriptWrappableVisitor* visitor) const {}
^
override
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{47:77-47:77}:" override"
../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h:59:27: warning: [chromium-style] Overriding method must be marked with 'override' or 'final'.
~V8IdleTask() = default;
^
override
fix-it:"../../third_party/WebKit/Source/core/dom/ScriptedIdleTaskController.h":{59:27-59:27}:" override"
,
Oct 24 2017
The following revision refers to this bug: https://chromium.googlesource.com/chromium/src.git/+/1d514493d6354b2d929e63ade437931cf82d6329 commit 1d514493d6354b2d929e63ade437931cf82d6329 Author: Daniel Cheng <dcheng@chromium.org> Date: Tue Oct 24 17:49:53 2017 clang plugin: improve handling of virtual / override fix it hints Removing virtual is tricky because the AST does not provide the location of the virtual keyword. Similarly, the AST also does not expose the location of '=' in |void F() = 0| or |void ~T() = default|. The plugin previously tries to hack around this in two different ways: 1. for virtual, it assumes the token at |method->getLocStart()| is the token that needs to be removed. 2. for override, it scans tokens backwards to try to find a ')' or '=', and guesses the appropriate insertion point. These heuristics break down sometimes, such as when a destructor is defaulted. This patch cleans up the token scanning logic so it works for the aforementioned case, and uses the same logic for determining removal of virtual as well. This also fixes the plugin to: 1. correctly prioritize retaining the 'final' specifier over the 'override' specifier 2. when generating the 'override' specifier fixit, also generates a removal fixit for 'virtual' if needed so cleanups don't reuqire two passes Bug: 777575 Change-Id: If7f9df09d3475b4e54596610f2eb5fb477e5cc1a Reviewed-on: https://chromium-review.googlesource.com/735079 Commit-Queue: Daniel Cheng <dcheng@chromium.org> Reviewed-by: Nico Weber <thakis@chromium.org> Cr-Commit-Position: refs/heads/master@{#511206} [modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/FindBadConstructsConsumer.cpp [modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/FindBadConstructsConsumer.h [modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/tests/blacklisted_dirs.txt [modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/tests/overridden_methods.cpp [modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/tests/overridden_methods.h [modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/tests/overridden_methods.txt [modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/tests/virtual_base_method_also_final.cpp [modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/tests/virtual_base_method_also_final.txt [modify] https://crrev.com/1d514493d6354b2d929e63ade437931cf82d6329/tools/clang/plugins/tests/virtual_specifiers.txt
,
Oct 24 2017
|
||
►
Sign in to add a comment |
||
Comment 1 by dcheng@chromium.org
, Oct 23 2017