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

Add @attribute syntax #1365

Merged
merged 4 commits into from Dec 15, 2012
Merged

Add @attribute syntax #1365

merged 4 commits into from Dec 15, 2012

Conversation

jacob-carlborg
Copy link
Contributor

Don't know if this is the best way to implement this, but it works and was a small change. I've run the DMD tests on Mac OS X 32bit and they all pass.

@deadalnix
Copy link
Contributor

Looked at the code and it does make sense to me.

nextToken();

Expressions* arguments = new Expressions();
Expression* arg = parseAssignExp();
Copy link
Member

Choose a reason for hiding this comment

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

This will allow things like
@A+b
It would be better to have:
e = parsePrimaryExp();
if (token.value == TOKlparen)
e = new CallExp(loc, e, parseArguments());

@jacob-carlborg
Copy link
Contributor Author

Walter: I've made the changes you mentioned in the comments.

stc = parseAttribute();
goto Lstc;
}

Copy link
Member

Choose a reason for hiding this comment

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

Why the space?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The empty line at line 390? I like to code with some extra space and empty newlines. I think it makes the code more readable if it's not packed.

Copy link
Member

Choose a reason for hiding this comment

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

Sure, but before an else block...?

Copy link

Choose a reason for hiding this comment

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

You need to conform to the existing style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then I guess I should use variable names with a single character, because that is soooo much clearer. Oh, and I must not forget to have code on the same line as an opening brace.

Copy link
Member

Choose a reason for hiding this comment

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

yes please

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not introducing more funky code. I'm trying to make the existing code better, more readable. Since there are now written guidelines/style guides it seems quite arbitrary what style to follow from the existing code.

But sure, I can play ball and can conform to the existing "style". Is it anything else that need to be changed than the extra newline?

Copy link

Choose a reason for hiding this comment

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

How does a blank line between if/else make code more readable? It's completely bizarre.

Copy link
Member

Choose a reason for hiding this comment

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

I agree with @AndrejMitrovic. At some level we must have a "tribal" shared view that goes beyond the written rules, and there will always be a lot that's not caught by the written rules but is silently agreed upon as not good. Our hope with keeping the written rules lax is to make for fewer, not more, discussions like this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Then drop it, I said I will remove the newline. Again, is there anything else I need to change?

@MartinNowak
Copy link
Member

The code in isPredefinedAttribute is partly redundant to parseAttribute.
Maybe I've missed the discussion (any ref?) but why do user attributes parse differently than predefined attributes?

@jacob-carlborg
Copy link
Contributor Author

The code in isPredefinedAttribute is partly redundant to parseAttribute.

Yes, isPredefinedAttribute is a bit redundant. Preferably I would have liked to put my changes in parseAttribute. But the function would need to return a different type for UDA.

Maybe I've missed the discussion (any ref?) but why do user attributes parse differently than predefined attributes?

Predefined attributes only allow a symbol. UDA allows arbitrary expressions: @(1+ 2) int a;. My changes allow basiclly function calls without the parentheses: @foo(3, 4) int a;

@MartinNowak
Copy link
Member

There is an obvious issue then.

struct safe { int val; }
@safe(0) int a;

@MartinNowak
Copy link
Member

I'd like to see more UDA unit test.
There should be tests with storage class modifiers and predefined attributes, also the postfix ones.
There should tests for failing parses.
There should tests for failing semantics.

@MartinNowak
Copy link
Member

Predefined attributes only allow a symbol.

OK, got that.
But why can't I have void foo() @safe @bar {}?
The postfix stuff is not related to this pull though.

@WalterBright
Copy link
Member

There is an obvious issue then.

Right, and this was discussed on the n.g. The most pragmatic solution was simply "no, you cannot have a user defined attribute named safe, nothrow, etc.".

@MartinNowak
Copy link
Member

The most pragmatic solution was simply "no, you cannot have a user defined attribute named safe, nothrow, etc.".

Yes it makes sense to treat those as reserved after an @ but the parser detect it and print an appropriate error message.

@andralex
Copy link
Member

It would be a lot better if these didn't have built-in status, and instead appeared in object.di.

@jacob-carlborg
Copy link
Contributor Author

It would be a lot better if these didn't have built-in status, and instead appeared in object.di.

I would guess that is quite a lot bigger change than this one. The compiler still need to know about them.

nextToken();

Expressions* exprs = new Expressions();
Expression* expr = parsePrimaryExp();
Copy link
Contributor

Choose a reason for hiding this comment

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

This parsePrimaryExp call would accept @a => a.
I'm not sure this is correct or not.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think it's cool that you can use a function as an attribute, or rather what the function returns:

string foo ()
{
    return "bar";
}

@foo int a;

static assert(__traits(getAttributes, a)[0] == "bar");

void main () { }

Perhaps lambdas can be useful as well.

Copy link
Contributor

Choose a reason for hiding this comment

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

@foo int a;

This is a irrelevant. That is an issue that UDA should interpret given function, or not.

I think that the parenthesis-less UDA should work syntactically as like the parenthesis omission in template instantiation.

Template Templ(alias a) { ... }
alias X1 = Templ!(a => a);   // OK
alias X2 = Templ!a => a;     // NG
@(a => a) void foo1() {} // OK
@a => a void foo() {}    // should be NG, and it is consistent with an illegal in X2

Copy link
Contributor

Choose a reason for hiding this comment

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

But, we already think @xyz(args) void foo() is legal, so we should think the syntax around UDA more seriously.

@jacob-carlborg
Copy link
Contributor Author

I've removed the empty newlines.

WalterBright added a commit that referenced this pull request Dec 15, 2012
@WalterBright WalterBright merged commit 15389cf into dlang:master Dec 15, 2012
@jacob-carlborg
Copy link
Contributor Author

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
7 participants