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

Protection trait #856

Merged
merged 2 commits into from
Nov 11, 2012
Merged

Protection trait #856

merged 2 commits into from
Nov 11, 2012

Conversation

adamdruppe
Copy link
Contributor

if (dim != 1)
goto Ldimerror;
PROT protection = PROTundefined;
Object *o = args->tdata()[0];
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Walter will get mad if you use tdata!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Finally looking at this again!) What should it be? Most the other code uses this same member. (I started this by copying the " else if (ident == Id::identifier)" branch)

@MartinNowak
Copy link
Member

ping

@adamdruppe
Copy link
Contributor Author

I've been very busy the last several weeks and intend to get back
to all this stuff probably after the 15th of this month.

@andralex
Copy link
Member

This is a useful trait. Ping @adamdruppe.

@adamdruppe
Copy link
Contributor Author

I went through and added some comments a while ago about details... I don't know how to solve all the comments' concerns myself.

@adamdruppe
Copy link
Contributor Author

I just pushed up some new code. I think this covers all the bases - both kinds of dot expressions and, of course, symbols. I also updated the style to match dmd git.

There's still the duplication of the protection names though. I just wanted to do one thing at a time though, we could DRY that in a separate little pull.

@ghost
Copy link

ghost commented Sep 26, 2012

Considering we have the ParameterStorageClass/FunctionAttribute enums I think we should also introduce a getProtection template in std.traits which would return an enum. Internally it could do string comparisons via __traits(getProtection).

@adamdruppe
Copy link
Contributor Author

Yeah, that'd be easy enough, though we'll sadly have to find names other that "public" etc. thanks to keywords. Ugh. But the phobos change can happen after the compiler is updated.

@ghost
Copy link

ghost commented Sep 27, 2012

Yup. I typically use uppercase names to avoid clashes. ParameterStorageClass uses underscores, maybe we should use that for consistency.

@ghost
Copy link

ghost commented Sep 27, 2012

Btw this pull will fix an undiscovered bug in std.typecons:

module inter;

interface Inter
{
    protected void test();
}
module test;

import inter;
import std.typecons;

void main()
{
    auto o = new BlackHole!Inter;
    o.test();  // works, o.test was declared public
}

BlackHole will be able to set the right protection attribute and disallow the above from compiling. I'd imagine this pull will be welcome for these sorts of templates that deal with classes.

@andralex
Copy link
Member

Great. Could you please file that so we keep track of it? Thanks!

@ghost
Copy link

ghost commented Sep 27, 2012

@adamdruppe
Copy link
Contributor Author

Hmm, the autotester is saying a test fails, but it works on my box, and I don't think my changes here could be responsible anyway.

http://d.puremagic.com/test-results/pull-history.ghtml?repoid=1&pullid=856

It looks like it was passing until last night though, and I haven't changed anything.... is this something I should worry about?

@braddr
Copy link
Member

braddr commented Sep 27, 2012

The master is broken, so it's not shocking that all pulls on top of that are also broken. It doesn't happen often, but when it does, it's annoying. Glare at @WalterBright for checking in broken code.

DotVarExp *dv = (DotVarExp *)e;
s = dv->var->isDeclaration();
}
else if (e->op == TOKvar)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is redundant code, because getDsymbol catches TOKvar.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

On Sat, Sep 29, 2012 at 01:39:48PM -0700, Hara Kenji wrote:

This is redundant code, because getDsymbol catches TOKvar.

ok, removed. The DotVar one is still needed to pass the test using
traits(getMember) though.

@9rnsr
Copy link
Contributor

9rnsr commented Sep 29, 2012

OK, now implementation looks good to me.
But, this is a language enhancement, and there is another pull request #952 for the design of protection traits feature.
So I can't merge this in my authority.
@WalterBright and @andralex , I'd like to assign this to you!

@dnadlinger
Copy link
Member

Maybe squash the commits together?

@adamdruppe
Copy link
Contributor Author

On Sat, Sep 29, 2012 at 02:51:27PM -0700, David Nadlinger wrote:

Maybe squash the commits together?

I have no idea how to do that.....

@yebblies
Copy link
Member

yebblies commented Oct 7, 2012

I have no idea how to do that.....

http://gitready.com/advanced/2009/02/10/squashing-commits-with-rebase.html

@adamdruppe
Copy link
Contributor Author

eh i tried it and think got some success, but it was complaining about merge conflicts and rejected non-fast-forward and other stuff. But it still builds so I'm gonna leave it here.

@nazriel
Copy link
Contributor

nazriel commented Oct 7, 2012

@adamdruppe if you are on IRC I can help you with it

@adamdruppe
Copy link
Contributor Author

On Sun, Oct 07, 2012 at 01:22:29PM -0700, Damian Ziemba wrote:

@adamdruppe if you are on IRC I can help you with it

I probably will be later today. Thanks!

@adamdruppe
Copy link
Contributor Author

I read some more about the git rebase and think I got it right this time. The diff looks clean now... we should be good to go.

// which returns a dot expression rather than a symbol
if(o->dyncast() == DYNCAST_EXPRESSION)
{
Expression* e = (Expression*) o;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Expression* e = (Expression*) o;
->
Expression *e = (Expression *)o;

@adamdruppe
Copy link
Contributor Author

We now have two commits: one is the protection trait, all changes squashed into one, and the other is removing some duplication on the protection names, by moving json.c's impl into a global area.

This should cover everyone's concerns.

@adamdruppe
Copy link
Contributor Author

Any chance we can get this pulled for the next release?

WalterBright added a commit that referenced this pull request Nov 11, 2012
@WalterBright WalterBright merged commit 913b485 into dlang:master Nov 11, 2012
WalterBright added a commit that referenced this pull request Nov 11, 2012
WalterBright added a commit that referenced this pull request Nov 11, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants