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 12653 - Implement the getFunctionAttributes trait. #3501

Merged
merged 1 commit into from Apr 27, 2014
Merged

Issue 12653 - Implement the getFunctionAttributes trait. #3501

merged 1 commit into from Apr 27, 2014

Conversation

ghost
Copy link

@ghost ghost commented Apr 26, 2014

https://issues.dlang.org/show_bug.cgi?id=12653

The current std.traits.functionAttributes implementation is very complicated due to reliance on demangling to figure out the attributes of a function. Additionally it doesn't support const/immutable/inout/shared.

Although support for these could be added in the library it's IMO time to move this whole functionality into the compiler as a trait. It's simpler this way and will speed up compilation.

The same could be done later for ParameterStorageClassTuple which uses demangling as well.

@ghost
Copy link
Author

ghost commented Apr 26, 2014

The test-case is largely based on the existing std.traits.functionAttributes unittest block. If there are missed cases please let me know!

@@ -604,6 +604,65 @@ Expression *semanticTraits(TraitsExp *e, Scope *sc)
TupleExp *tup = new TupleExp(e->loc, udad ? udad->getAttributes() : new Expressions());
return tup->semantic(sc);
}
else if (e->ident == Id::getFunctionAttributes)
Copy link
Member

Choose a reason for hiding this comment

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

Could really use some documentation comments here. Note that there'll need to be a corresponding spec PR explaining this as well, the two should be fairly similar.

Copy link
Author

Choose a reason for hiding this comment

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

Note that there'll need to be a corresponding spec PR explaining this as well, the two should be fairly similar.

Yes, and since we already have getAttributes I'll make sure to document the difference between the two.

@ghost
Copy link
Author

ghost commented Apr 26, 2014

Updated everything except this block:

if (tf->isref) mods->push(new StringExp(Loc(), "ref"));
if (tf->isproperty) mods->push(new StringExp(Loc(), "@property"));
if (tf->isnothrow) mods->push(new StringExp(Loc(), "nothrow"));
if (tf->isnogc) mods->push(new StringExp(Loc(), "@nogc"));
if (tf->purity) mods->push(new StringExp(Loc(), "pure"));

I don't know where else the code uses tuples or arrays of these, otherwise I could refactor it to a common function. Well I guess I could do that preemptively anyway?

case TRUSTdefault:
break;

default: assert(0); // unhandled trust
Copy link
Member

Choose a reason for hiding this comment

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

yay!

@ghost ghost closed this Apr 26, 2014
@ghost
Copy link
Author

ghost commented Apr 26, 2014

Update: I've implemented a generic foreachAttributes in TypeFunction which takes a callback. Maybe we could do the same with type modifiers (MODconst, etc.).

Let me know what you think @WalterBright.

@ghost
Copy link
Author

ghost commented Apr 26, 2014

Documentation/spec pull made: dlang/dlang.org#566

@ghost
Copy link
Author

ghost commented Apr 26, 2014

Just noticed this doesn't work with function templates:

void main()
{
    int sum(T1, T2)(T1 t1, T2 t2) { return t1 + t2; }
    pragma(msg, __traits(getFunctionAttributes, sum));  // fail
}

Should it work?

@WalterBright
Copy link
Member

Should it work?

No. It's not a function until it is instantiated. In fact, the attributes cannot be determined until it is instantiated, because attributes are inferred on function templates.

trustToBuffer(buf, t->trust);
}

t->foreachAttributes(&buf, preAppendStrings);
Copy link
Member

Choose a reason for hiding this comment

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

I think this is a nice improvement. Great! It can go a little further. We can sort of create lambda functions in C++ using a little trick - local structs with member functions. This trick is done elsewhere in dmd, such as ParamUnique and ParamExact in func.c.

Copy link
Author

Choose a reason for hiding this comment

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

I'll have a look and fix it up.

@ghost ghost closed this Apr 27, 2014
@ghost ghost reopened this Apr 27, 2014
@ghost
Copy link
Author

ghost commented Apr 27, 2014

Implemented attributesApply and modifiersApply. I didn't add a this parameter like in overloadAppy since it wasn't really needed here.

WalterBright added a commit that referenced this pull request Apr 27, 2014
Issue 12653 - Implement the getFunctionAttributes trait.
@WalterBright WalterBright merged commit b65f13e into dlang:master Apr 27, 2014
@ghost
Copy link
Author

ghost commented Apr 27, 2014

@WalterBright: I seem to have forgotten to support typeof(func), IOW:

struct S2
{
    int pure_const() const pure { return 0; }
}

static assert(__traits(getFunctionAttributes, typeof(S2.pure_const)) == tuple!("const", "pure"));

I can add a fixup for this. However I've noticed that const/immutable/inout/shared are not visible when using typeof, even though things like purity are visible. So the results will be different. I'm not sure whether this is a new bug?

Edit: Nevermind.

@ghost
Copy link
Author

ghost commented Apr 27, 2014

Nevermind, that is not a bug. typeof() means you don't have a function declaration, so const/inout/etc are naturally missing.

@ghost ghost mentioned this pull request Apr 27, 2014
@MartinNowak
Copy link
Member

Nice, so we can replace the mangling based implementation of std.traits.functionAttributes now?

@ghost
Copy link
Author

ghost commented May 5, 2014

Nice, so we can replace the mangling based implementation of std.traits.functionAttributes now?

First this needs to be pulled, and then the Phobos pull to implement just that is here.


// const/immutable/inout/shared is only valid for member functions
if (fd)
fd->type->modifiersApply(&pa, &PushAttributes::fp);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why not print member function attributes (const, immutable, etc) when the argument is a type?

struct S { void foo() const {} }
pragma(msg, __traits(getFunctionAttributes,        S.foo ));  // tuple("const")
pragma(msg, typeof(S.foo));  // const void()
pragma(msg, __traits(getFunctionAttributes, typeof(S.foo)));  // tuple(), inconsistent

Copy link
Contributor

Choose a reason for hiding this comment

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

Note that, typeof(S.foo) == 'const void()' == 'const(void())' == 'void() const' internally.

Copy link
Author

Choose a reason for hiding this comment

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

Why not print member function attributes (const, immutable, etc) when the argument is a type?

I don't know how to get from a TypeFunction to a FuncDeclaration, which holds these attributes. I would actually appreciate some help here. If you can improve getFunctionAttributes further it would be awesome!

Copy link
Contributor

Choose a reason for hiding this comment

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

These attributes are in the mod field of TypeFunction. So if the TypeFunction comes from member function, you can access them without fd.

Copy link
Author

Choose a reason for hiding this comment

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

Oh, right! I will fixup, thanks.

int pureF() pure { return 0; }
}

static assert(tupleLength!(__traits(getFunctionAttributes, S.noF)) == 0);
Copy link
Contributor

Choose a reason for hiding this comment

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

S.noF is a system function, so __traits(getFunctionAttributes) should list it?

Copy link
Author

Choose a reason for hiding this comment

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

Ah, I didn't notice this. So if trust is set to TRUSTdefault this implies system, I didn't actually check the enum even though it describes this:

enum TRUST
{
    TRUSTdefault = 0,
    TRUSTsystem = 1,    // @system (same as TRUSTdefault)
    TRUSTtrusted = 2,   // @trusted
    TRUSTsafe = 3,      // @safe
};

Although I'm not sure why an explicit TRUSTdefault was even added.

Copy link
Author

Choose a reason for hiding this comment

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

I can fix this up, but I'm unsure about extracting const/immutable via typeof(func).

Copy link
Author

Choose a reason for hiding this comment

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

Made pull: #3533

@yebblies
Copy link
Member

yebblies commented May 8, 2014

Although I'm not sure why an explicit TRUSTdefault was even added.

So you can tell the difference between 'trust not specified' and 'trust specified as system' when deciding to do inference.

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