Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Issue 11616 - Introduce virtual keyword #2895

Merged
1 commit merged into from Feb 25, 2014

Conversation

Projects
None yet
8 participants
@yebblies
Copy link
Member

commented Nov 28, 2013

{
final override void a() {}
final override void b() {}
final override void d() {}

This comment has been minimized.

Copy link
@ghost

ghost Nov 28, 2013

I don't understand this, you're overriding a final method?

This comment has been minimized.

Copy link
@mihails-strasuns

mihails-strasuns Nov 28, 2013

virtual is in conflict with final and last applied should be active. Similar to:

@safe:
    @system void foo();

This comment has been minimized.

Copy link
@yebblies

yebblies Nov 28, 2013

Author Member

Exactly!

This comment has been minimized.

Copy link
@ghost
@Dgame

This comment has been minimized.

Copy link

commented Dec 1, 2013

Comes with 2.065?

@ghost

This comment has been minimized.

Copy link

commented Dec 1, 2013

@Dgame: We don't know yet.

---
fail_compilation/fail11616.d(22): Error: function fail11616.D.a cannot override final function fail11616.C.a
fail_compilation/fail11616.d(22): Error: function fail11616.D.a does not override any function
fail_compilation/fail11616.d(23): Error: function fail11616.D.b cannot override final function fail11616.C.b

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Dec 3, 2013

Contributor

The errors on line 6 and 7 here confuse me.
function fail11616.D.a does not override any function, seems to be wrong, since it does attempt to override a function. Isn't the first error sufficient?
function fail11616.D.b cannot override final function fail11616.C.b, I don't think this is quite a correct account of the error, since override is not stated anywhere.

This comment has been minimized.

Copy link
@yebblies

yebblies Dec 4, 2013

Author Member

6: Probably.
7: override is checked later, by default functions with the same signature will override.

This comment has been minimized.

Copy link
@TurkeyMan

TurkeyMan Dec 4, 2013

Contributor

7: regardless, is that correct though? I don't think the error message will particularly help someone fix the problem.
I understand the convenience aspect in the compiler code, but error messages should be useful.

@WalterBright

This comment has been minimized.

Copy link
Member

commented Dec 3, 2013

Not for 2.065 - which is for bug fixes and stability.

@ghost

This comment has been minimized.

Copy link

commented Feb 4, 2014

The pull is incredibly simple. LGTM. And this is just the first step, the next step is warnings/deprecation about overrides on non-final methods (which used to be virtual by default and won't be anymore).

@Dgame

This comment has been minimized.

Copy link

commented Feb 20, 2014

Can hardly wait for it... :)

@ghost

This comment has been minimized.

Copy link

commented Feb 24, 2014

@yebblies: If you can rebase this I'll merge. We de-facto already have an approval for it, waiting would just delay the inevitable.

Issue 11616 - Introduce virtual keyword
Introduce the virtual keyword
@Dgame

This comment has been minimized.

Copy link

commented Feb 25, 2014

All green. What is the next step?

@ghost

This comment has been minimized.

Copy link

commented Feb 25, 2014

All green. What is the next step?

The next step is for someone to merge this and get all the flak for what has already become an accepted path forward. I can already tell there's going to be controversy. Anyway, shields up!.

@ghost

This comment has been minimized.

Copy link

commented Feb 25, 2014

Auto-merge toggled on

ghost pushed a commit that referenced this pull request Feb 25, 2014

Merge pull request #2895 from yebblies/issue11616
Issue 11616 - Introduce virtual keyword

@ghost ghost merged commit 28acc4a into dlang:master Feb 25, 2014

1 check passed

default Pass: 10
Details
@9rnsr

This comment has been minimized.

Copy link
Member

commented Feb 26, 2014

Honestly I didn't participate the discussion (I'm now starting to read http://forum.dlang.org/thread/yzsqwejxqlnzryhrkfuq@forum.dlang.org), but why new virtual keyword and change the behavior to 'fianl-by-default' is necessary?
I can easyly think a new attribute syntax without adding keyword.

class C1
{
default(final):
    void foo() {}

default(virtual):   // 'virtual' is not a keyword
    void bar() {}
}

class D1 : C1
{
    void foo() {}   // hides C1.foo, but don't override it
    override void bar() {}
}

And, for optimizing vtbl, specifying the attribute on the root class would be enough.

default(final) class C2 {}
//class D2 : C2 {}    // error, force to be annotated with @default(final).
default(final) class D2 : C2 {}    // OK.

I think DIP51 and issue is an unnecessary huge breaking.

@yebblies

This comment has been minimized.

Copy link
Member Author

commented Feb 26, 2014

I think DIP51 and issue is an unnecessary huge breaking.

Bit late to the party.

// 'virtual' is not a keyword

I don't think this is something worth worrying about.

@yebblies yebblies deleted the yebblies:issue11616 branch Feb 26, 2014

@9rnsr

This comment has been minimized.

Copy link
Member

commented Feb 26, 2014

I opened #3337.

@9rnsr

This comment has been minimized.

Copy link
Member

commented Feb 26, 2014

Bit late to the party.

I recognize, but the way to implement things is still debatable.

I don't think this is something worth worrying about.

Why? I know the discussion about body keyword. For some ported code from C++, making virtual keyword would not be a problem. However some D native coders may already use virtual for the variable names. I think they have same weight.

@Orvid

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2014

My (poorly described, due to how late at night it is here) opinion on 9rnsr's view simply this: Why use some horrid syntax to get around adding a keyword? This is a one-time update for code, with a simple and elegant syntax change to fix something that I think should never have been done the way it is in the first place. Backwords compatibility (in my mind) is the single biggest reason C++'s syntax is so convoluted, because they went to extremes to avoid breaking existing code.

@TurkeyMan

This comment has been minimized.

Copy link
Contributor

commented Feb 26, 2014

Not to mention that in plenty of other instances, compatibility with C++ is a stated goal and preference, and in this case, I think it makes absolute sense to match C++ here. Porting C++ code will certainly be simplified if the keywords mean the same thing.

@ghost

This comment has been minimized.

Copy link

commented Feb 26, 2014

I've yet to see someone name a variable virtual. And using virtual makes it easier for C/C++ programmers to understand D and makes it easier to port code. Why invent completely bizarre new syntax like default(virtual):?

@yebblies

This comment has been minimized.

Copy link
Member Author

commented Feb 28, 2014

I've yet to see someone name a variable virtual.

Actually, this change broke magicport because I had a variable named virtual. It took a couple of seconds to rename it to isvirtual - problem solved.

@andralex

This comment has been minimized.

Copy link
Member

commented Mar 9, 2014

Walter "got browbeat" into accepting this but has misgivings about the end game here. He hasn't had a chance to review this pull. I've never approved the feature.

Could we please make sure language changes are pulled by Walter or myself? Also please let's not take more steps on this for the time being. Thanks.

@ghost

This comment has been minimized.

Copy link

commented Mar 9, 2014

Walter "got browbeat" into accepting this but has misgivings about the end game here. He hasn't had a chance to review this pull. I've never approved the feature.

After many discussions he seemed to have accepted it: forum thread.

@andralex

This comment has been minimized.

Copy link
Member

commented Mar 9, 2014

@AndrejMitrovic that is correct. He actually does not believe this is the right move though (same as me).

@ghost

This comment has been minimized.

Copy link

commented Mar 9, 2014

You mean the whole idea of virtual or just the way we're going forward with this? If the latter, what's the plan?

@andralex

This comment has been minimized.

Copy link
Member

commented Mar 9, 2014

The whole idea of virtual.

@yebblies

This comment has been minimized.

Copy link
Member Author

commented Mar 10, 2014

I'm hoping Walter (and you, @andralex) will see the light after seeing the 1000s of finals plastered everywhere in the D version of the frontend. With this change I can simply put final: at the top and stick a virtual in front of the couple of virtual functions.

@andralex

This comment has been minimized.

Copy link
Member

commented Mar 16, 2014

@yebblies: we are converging on a consensus around final(false) with an eye for extending things further later. Would you be okay with that? Thanks!

dnadlinger added a commit to dnadlinger/dmd that referenced this pull request Jun 14, 2014

Remove virtual keyword.
It was added in GitHub pull request dlang#2895, but since fell
into disfavor compared to a more general way of negating
storage classes.

This issue was closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.