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 1673 - Implement the isTemplate trait. #3380

Merged
merged 1 commit into from Aug 8, 2014
Merged

Issue 1673 - Implement the isTemplate trait. #3380

merged 1 commit into from Aug 8, 2014

Conversation

ghost
Copy link

@ghost ghost commented Mar 14, 2014

http://d.puremagic.com/issues/show_bug.cgi?id=1673

A rework of #1239, this time implementing an isTemplate trait.

@9rnsr @yebblies: Are the two checks in bool isTemplate(Dsymbol *s) good enough?

@yebblies
Copy link
Member

Are the two checks in bool isTemplate(Dsymbol *s) good enough?

No idea, sorry. Kenji will know.

@@ -200,6 +201,22 @@ Expression *isDeclX(TraitsExp *e, bool (*fp)(Declaration *d))
return new IntegerExp(e->loc, result, Type::tbool);
}

Expression *isSymbolX(TraitsExp *e, bool (*fp)(Dsymbol *s))
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure this is worth putting in a function for only one use.

Copy link
Author

Choose a reason for hiding this comment

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

I was thinking maybe future code might have a use for it. I'm fine either way. Should I remove it?

Copy link
Member

Choose a reason for hiding this comment

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

Nah I don't feel that strongly about it.

@yebblies
Copy link
Member

I can't wait to delegatize this damn file.

@9rnsr
Copy link
Contributor

9rnsr commented Mar 15, 2014

Are the two checks in bool isTemplate(Dsymbol *s) good enough?

Template definitions can be overloaded with functions. So do you think how __traits(isTemplate) should work for the cases?

void foo() {}
void foo(T)(T) {}
enum bool x = __traits(isTemplate, foo);

Current code will return false with the case. To fix it, you can use overloadApply function.

@ghost
Copy link
Author

ghost commented Mar 15, 2014

@9rnsr: Thanks! Updated the code and the test-case, let me know if I've missed something.

if (p->found)
return 0; // found a template

if (s->isTemplateDeclaration() || s->toAlias()->isTemplateDeclaration())
Copy link
Contributor

Choose a reason for hiding this comment

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

toAlias case is unnecessary because it is already handled in overloadApply

Copy link
Author

Choose a reason for hiding this comment

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

Thanks! Btw, is there any way to make fpisTemplate stop the overloadApply early? I'm using a if (p->found) check to return 0 early, but this doesn't stop the overloadApply.

Copy link
Author

Choose a reason for hiding this comment

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

I guess I could return 1?

@ghost
Copy link
Author

ghost commented Mar 15, 2014

Ah I think I can implement this in a simpler way:

static int fpisTemplate(void *param, Dsymbol *s)
{
    if (s->isTemplateDeclaration())
        return 1;

    return 0;
}

// edited
bool isTemplate(Dsymbol *s)
{
    if (s->isTemplateDeclaration() || s->toAlias()->isTemplateDeclaration())
        return true;

    return overloadApply(s, NULL, &fpisTemplate);
}

/*
bool isTemplate(Dsymbol *s)
{
    if (s->isTemplateDeclaration() || s->toAlias()->isTemplateDeclaration())
        return true;

    FuncDeclaration *f = s->isFuncDeclaration();
    if (!f) return false;

    return overloadApply(f, NULL, &fpisTemplate);
}
*/

This seems to work locally. @9rnsr what do you think?

Edit: Updated with your latest comment.

@ghost
Copy link
Author

ghost commented Mar 15, 2014

It looks like I can even use this:

bool isTemplate(Dsymbol *s)
{
    /*
    if (s->isTemplateDeclaration() || s->toAlias()->isTemplateDeclaration())
        return true;
    */

    return overloadApply(s, NULL, &fpisTemplate);
}

template isTemplate(alias T)
{
enum bool isTemplate = __traits(isTemplate, T);
}
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't like to use wrapper template in the compiler test suite. This test suite tests only the case that __traits(isTemplate) gets an aliased symbol T.

Copy link
Author

Choose a reason for hiding this comment

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

Ok, will fix.

@9rnsr
Copy link
Contributor

9rnsr commented Mar 15, 2014

More better is:

bool isTemplate(Dsymbol *s)
{
    return overloadApply(s, NULL, &fpisTemplate) != 0;
}

!= 0 would be necessary for DDMD conversion.

@ghost
Copy link
Author

ghost commented Mar 15, 2014

More better is:

If I remove those calls I get errors:

static int fpisTemplate(void *param, Dsymbol *s)
{
    if (s->isTemplateDeclaration())
        return 1;

    return 0;
}

bool isTemplate(Dsymbol *s)
{
    /* if (s->isTemplateDeclaration() || s->toAlias()->isTemplateDeclaration())
        return true;

    FuncDeclaration *f = s->isFuncDeclaration();
    if (!f) return false; */

    return overloadApply(s, NULL, &fpisTemplate) != 0;
}

Output:

test.d(17): Error: template instance test1673.Foo!int is aliased to a function
test.d(21): Error: template instance test1673.Bar!int is aliased to a function
test.d(23): Error: template instance test1673.Bar!int.Doo!int is aliased to a function
test.d(17): Error: template instance test1673.Foo!int is aliased to a function
test.d(21): Error: template instance test1673.Bar!int is aliased to a function
test.d(23): Error: template instance test1673.Bar!int.Doo!int is aliased to a function

I seem to need both commented-out sections in order for the test-suite to pass. Test-suite:

module test1673;

template Foo(T...) { }

template Bar(T...)
{
    template Doo(T...)
    {
    }
}

template Tuple(T...) { alias Tuple = T; }

void main()
{
    static assert( __traits(isTemplate, Foo));
    static assert(!__traits(isTemplate, Foo!int));
    static assert(!__traits(isTemplate, main));

    static assert( __traits(isTemplate, Bar));
    static assert(!__traits(isTemplate, Bar!int));
    static assert( __traits(isTemplate, Bar!(int).Doo));
    static assert(!__traits(isTemplate, Bar!(int).Doo!int));

    alias Tup = Tuple!(Foo, Foo!int, Bar, Bar!int, Bar!(int).Doo, Bar!(int).Doo!int);

    static assert( __traits(isTemplate, Tup[0]));
    static assert(!__traits(isTemplate, Tup[1]));
    static assert( __traits(isTemplate, Tup[2]));
    static assert(!__traits(isTemplate, Tup[3]));
    static assert( __traits(isTemplate, Tup[4]));
    static assert(!__traits(isTemplate, Tup[5]));
}

/// test overloads
void foo_over() { }
void foo_over(T : int)(T) { }
void foo_over(T : float)(T) { }
static assert(__traits(isTemplate, foo_over));

/// ditto
void bar_over() { }
void bar_over(int) { }
static assert(!__traits(isTemplate, bar_over));

/// alias to overloads
alias a_foo_over = foo_over;
static assert(__traits(isTemplate, a_foo_over));

/// ditto
alias a_bar_over = bar_over;
static assert(!__traits(isTemplate, a_bar_over));

@9rnsr
Copy link
Contributor

9rnsr commented Mar 15, 2014

Add this code at the start of isTemplate function would fix the issue:

if (!s->toAlias()->isOverloadable())
    return false;

@ghost
Copy link
Author

ghost commented Mar 15, 2014

@9rnsr: Excellent, thanks! Updated, let's wait for green.

@ghost
Copy link
Author

ghost commented Mar 15, 2014

It's green now. Hopefully this should do it? I'll write a spec update soon if this is a go.

@mihails-strasuns
Copy link

Please add test case with checking templated function symbol (eponymous templates are distinct enough)

@ghost
Copy link
Author

ghost commented Mar 15, 2014

@Dicebot: I thought foo_over covered it? What did you have in mind?

@mihails-strasuns
Copy link

Nevermind, just being blind :)

@ghost
Copy link
Author

ghost commented Mar 17, 2014

It's been green for a while. Ping @9rnsr

@ghost
Copy link
Author

ghost commented Mar 21, 2014

Ping-pong. If it's good to go auto-merge should be set. It's a top-wanted feature after all. :>

@ghost
Copy link
Author

ghost commented Apr 27, 2014

@WalterBright ping for review / approval (Andrei approved already). I'll write a spec pull shortly.

@ghost
Copy link
Author

ghost commented May 9, 2014

Rebased.

@quickfur
Copy link
Member

ping
Autotester has been passing for a while now. Are we just waiting for Walter to approve this?

@ghost ghost added Enhancement labels Aug 4, 2014
@AndrewEdwards AndrewEdwards added this to the 2.067 milestone Aug 7, 2014
@andralex
Copy link
Member

andralex commented Aug 8, 2014

ping - what's the status of this?

@andralex
Copy link
Member

andralex commented Aug 8, 2014

Well I think I'll just merge this. "What's the worst that could happen?"

andralex added a commit that referenced this pull request Aug 8, 2014
Issue 1673 - Implement the isTemplate trait.
@andralex andralex merged commit c5b285d into dlang:master Aug 8, 2014
@ghost
Copy link
Author

ghost commented Aug 8, 2014

Thanks!!

ibuclaw pushed a commit to ibuclaw/dmd that referenced this pull request Jul 10, 2022
Remove a few uses of fully qualified names that rely on a DMD bug
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants