Add a few missing overrides found by a new clang warning.

Add a few missing overrides found by a new clang warning.

Namely, -Winconsistent-missing-override. No behavior change.

BUG=v8:3658
LOG=N
R=jkummerow@chromium.org

Committed: https://code.google.com/p/v8/source/detail?r=24994

[Nico, 2014-10-28 23:21:43]

thakis@chromium.org changed reviewers:
+ vegorov@chromium.org

[Vyacheslav Egorov (Google), 2014-10-28 23:23:57]

vegorov@google.com changed reviewers:
+ jkummerow@chromium.org
- vegorov@chromium.org

[Vyacheslav Egorov (Google), 2014-10-28 23:24:21]

vegorov@google.com changed reviewers:
+ vegorov@google.com

[Vyacheslav Egorov (Google), 2014-10-28 23:24:22]

-me +jkummerow

[Nico, 2014-10-29 00:48:12]

jkummerow: If you like this, it'd be great if you could land it too. I'm not a
v8 committer.

[Sven Panne, 2014-10-29 08:10:15]

If I understand the discussion on chromium-dev
(https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/VTNZzizN0zo)
correctly, there are 2 steps after this:

   * Replace "OVERRIDE" with "override" (and nuke our feature detection for
this).

   * Remove all "virtual"s when there is an "override", because the latter
implies the former (it's required by the Google3 style guide). But we can't do
that right now because cpplint/presubmit doesn't like it yet.

Correct?

[Jakob Kummerow, 2014-10-29 10:17:45]

LGTM. I'll land.

Michael: FYI (ast.h).

https://codereview.chromium.org/688533002/diff/1/src/ast.h
File src/ast.h (right):

https://codereview.chromium.org/688533002/diff/1/src/ast.h#newcode1716
src/ast.h:1716: virtual void SetFirstICFeedbackSlot(FeedbackVectorICSlot slot) {
The fact that we don't need OVERRIDE here smells buggy. mvstanton@ confirmed.
I'll fix before landing. (This is supposed to override SetFirstFeedbackICSlot.)

https://codereview.chromium.org/688533002/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/688533002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:119: virtual void
VisitDeclarations(ZoneList<Declaration*>* declarations);
Why don't we need OVERRIDE here? Oversight?

https://codereview.chromium.org/688533002/diff/1/src/full-codegen.h
File src/full-codegen.h (right):

https://codereview.chromium.org/688533002/diff/1/src/full-codegen.h#newcode395
src/full-codegen.h:395: void VisitDeclarations(ZoneList<Declaration*>*
declarations);
Another |VisitDeclarations| that should be both virtual and OVERRIDE.

https://codereview.chromium.org/688533002/diff/1/src/typing.h
File src/typing.h (right):

https://codereview.chromium.org/688533002/diff/1/src/typing.h#newcode75
src/typing.h:75: void VisitDeclarations(ZoneList<Declaration*>* declarations);
...and one more VisitDeclarations

[Nico, 2014-10-29 15:43:39]

Thanks! Let me know if you want me to edit the 3 lines you pointed out and the 1
I'm pointing out, or if you'll do that before landing. (I figured I'd miss some;
http://build.chromium.org/p/chromium.fyi/builders/Cr%20Win%20Clang/builds/122...
shows all these warnings and whatever I missed will be easier to see once this
set of fixes is rolled into chromium.)

svenpanne:
> If I understand the discussion on chromium-dev
>
(https://groups.google.com/a/chromium.org/forum/#!topic/chromium-dev/VTNZzizN0zo)
> correctly, there are 2 steps after this:
> 
>    * Replace "OVERRIDE" with "override" (and nuke our feature detection for
> this).
> 
>    * Remove all "virtual"s when there is an "override", because the latter
> implies the former (it's required by the Google3 style guide).

Correct. If v8 wants to require c++11 language features, then v8 can do this
too.

> But we can't do
> that right now because cpplint/presubmit doesn't like it yet.
> 
> Correct?

In chromium-land, the problem is that the plugin currently requires "virtual" on
virtual method. dcheng is fixing this. v8 doesn't use the chromium clang plugin
as far as I know, so this might not be a problem for you.

https://codereview.chromium.org/688533002/diff/1/src/compiler/ast-graph-build...
File src/compiler/ast-graph-builder.h (right):

https://codereview.chromium.org/688533002/diff/1/src/compiler/ast-graph-build...
src/compiler/ast-graph-builder.h:119: virtual void
VisitDeclarations(ZoneList<Declaration*>* declarations);
On 2014/10/29 10:17:45, Jakob wrote:
> Why don't we need OVERRIDE here? Oversight?

Yes, oversight.
https://code.google.com/p/v8/issues/attachmentText?id=3658&aid=36580003000&na...
mentions it. Do you want to add it before landing, or do you want me to upload
another patch set?

https://codereview.chromium.org/688533002/diff/1/src/full-codegen.h
File src/full-codegen.h (right):

https://codereview.chromium.org/688533002/diff/1/src/full-codegen.h#newcode395
src/full-codegen.h:395: void VisitDeclarations(ZoneList<Declaration*>*
declarations);
On 2014/10/29 10:17:45, Jakob wrote:
> Another |VisitDeclarations| that should be both virtual and OVERRIDE.

Yes (mentioned here:
https://code.google.com/p/v8/issues/attachmentText?id=3658&aid=36580003000&na...).
Same question as on the other.

https://codereview.chromium.org/688533002/diff/1/src/typing.h
File src/typing.h (right):

https://codereview.chromium.org/688533002/diff/1/src/typing.h#newcode75
src/typing.h:75: void VisitDeclarations(ZoneList<Declaration*>* declarations);
On 2014/10/29 10:17:45, Jakob wrote:
> ...and one more VisitDeclarations

Yes:
https://code.google.com/p/v8/issues/attachmentText?id=3658&aid=36580003000&na...

https://codereview.chromium.org/688533002/diff/1/src/typing.h#newcode76
src/typing.h:76: void VisitStatements(ZoneList<Statement*>* statements);
This one too:
https://code.google.com/p/v8/issues/attachmentText?id=3658&aid=36580003000&na...

[Jakob Kummerow, 2014-10-29 18:31:19]

Committed patchset #1 (id:1) manually as 24994 (presubmit successful).