Skip to content

Refactor duplicated code from ASTBase node AliasThis into a mixin template #12796

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.

Sign up for GitHub

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

Closed
RazvanN7 wants to merge 1 commit into from
Closed

Refactor duplicated code from ASTBase node AliasThis into a mixin template #12796

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 2 additions & 11 deletions src/dmd/aliasthis.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ module dmd.aliasthis;

import core.stdc.stdio;
import dmd.aggregate;
import dmd.astmembers;
import dmd.dscope;
import dmd.dsymbol;
import dmd.expression;
Expand All @@ -31,17 +32,12 @@ import dmd.visitor;
*/
extern (C++) final class AliasThis : Dsymbol
{
Identifier ident;
/// The symbol this `alias this` resolves to
Dsymbol sym;
/// Whether this `alias this` is deprecated or not
bool isDeprecated_;

extern (D) this(const ref Loc loc, Identifier ident)
{
super(loc, null); // it's anonymous (no identifier)
this.ident = ident;
}
mixin parseTimePropertiesAliasThis;
Copy link
Member

@ibuclaw ibuclaw Jul 1, 2021

Choose a reason for hiding this comment

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

This doesn't really address the duplication though, does it? There are still two copies of AliasThis present, and every time a new AST node is introduced it must still be defined twice.

Copy link
Contributor Author

@RazvanN7 RazvanN7 Jul 1, 2021

Choose a reason for hiding this comment

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

The only way we could eliminate the duplication would be to have the ASTCodegen classes inherit from the ASTBase classes, however, that would require multiple inheritance since a node must inherit its parse time counterpart but also its logical ancestor (for example AddExp would need to inherit ParseTimeAddExp and BinaryExp). This could technically be done by using inheritance and alias this, but I would rather not go there since this currently isn't defined behavior (and ideally alias this would be deprecated for classes).

Instead, this PR limits the scope of the duplication. Indeed, new AST node declarations still need to be duplicated, but I would argue that this doesn't happen very often.

I couldn't come up with a solution that doesn't require to refactor the compiler entirely (e.g. stripping all AST nodes of their functions, basically transforming them into data containers) . If anyone has any propositions, I'm open to suggestions. However, I doubt that a feasible solution to get rid of the duplicated AST node declarations is possible at this point.

Copy link
Member

@ibuclaw ibuclaw Jul 1, 2021

Choose a reason for hiding this comment

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

I couldn't come up with a solution that doesn't require to refactor the compiler entirely (e.g. stripping all AST nodes of their functions, basically transforming them into data containers) . If anyone has any propositions, I'm open to suggestions. However, I doubt that a feasible solution to get rid of the duplicated AST node declarations is possible at this point.

Why not? We've already removed the semantic, semantic2, semantic3, resolve, toIR, toCtype, toElem, toDt, and probably many more this way.

  1. There are pointless member functions:

    dmd/src/dmd/statement.d

    Lines 216 to 249 in 0ff19fb

    final bool usesEH()
    {
    extern (C++) final class UsesEH : StoppableVisitor
    {
    alias visit = typeof(super).visit;
    public:
    override void visit(Statement s)
    {
    }
    override void visit(TryCatchStatement s)
    {
    stop = true;
    }
    override void visit(TryFinallyStatement s)
    {
    stop = true;
    }
    override void visit(ScopeGuardStatement s)
    {
    stop = true;
    }
    override void visit(SynchronizedStatement s)
    {
    stop = true;
    }
    }
    scope UsesEH ueh = new UsesEH();
    return walkPostorder(this, ueh);
    }

  2. There doesn't need to be a big clear-out done, just anything that isn't to do with examining itself. e.g. from Statement:

    dmd/src/dmd/statement.h

    Lines 119 to 129 in 0ff19fb

    virtual Statement *getRelatedLabeled() { return this; }
    virtual bool hasBreak() const;
    virtual bool hasContinue() const;
    bool usesEH();
    bool comeFrom();
    bool hasCode();
    virtual Statement *scopeCode(Scope *sc, Statement **sentry, Statement **sexit, Statement **sfinally);
    virtual Statements *flatten(Scope *sc);
    virtual Statement *last();
    virtual ReturnStatement *endsWithReturnStatement() { return NULL; }

    Remove the above highlighted functions, and astbase can just publicly import dmd.statement...

Copy link
Contributor Author

@RazvanN7 RazvanN7 Jul 1, 2021

Choose a reason for hiding this comment

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

Unfortunately, it's not that easy. Looking at statement.d it imports a lot of modules that don't have anything to do with parsing. Before being able to import statement.d we need to get rid of all of those dependencies. That is no easy task and it requires a lot of refactorings.

Copy link
Member

@ibuclaw ibuclaw Jul 1, 2021

Choose a reason for hiding this comment

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

I'd be surprised if it stays that way after dealing with the above functions.

Copy link
Contributor Author

@RazvanN7 RazvanN7 Jul 2, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

The problem is that statement.d has many dependencies. Even if we move out everything that relies on semantic information from statement.d, we will still have some Expression fields. expression.d has other dependencies that rely on semantic information, so now we need to the same for it too etc. My point is that, before we can simply do import statement; in ASTBase, we need to do this cleanup for all the AST nodes, otherwise the import dependencies are going to drag semantic information into ASTBase.


override AliasThis syntaxCopy(Dsymbol s)
{
Expand All @@ -61,11 +57,6 @@ extern (C++) final class AliasThis : Dsymbol
return this;
}

override void accept(Visitor v)
{
v.visit(this);
}

override bool isDeprecated() const
{
return this.isDeprecated_;
Expand Down
15 changes: 2 additions & 13 deletions src/dmd/astbase.d
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
module dmd.astbase;

import dmd.astenums;
import dmd.astmembers;
import dmd.parsetimevisitor;

/** The ASTBase family defines a family of AST nodes appropriate for parsing with
Expand Down Expand Up @@ -256,19 +257,7 @@ struct ASTBase

extern (C++) class AliasThis : Dsymbol
{
Identifier ident;

extern (D) this(const ref Loc loc, Identifier ident)
{
super(null);
this.loc = loc;
this.ident = ident;
}

override void accept(Visitor v)
{
v.visit(this);
}
mixin parseTimePropertiesAliasThis;
}

extern (C++) final class AliasAssign : Dsymbol
Expand Down
18 changes: 18 additions & 0 deletions src/dmd/astmembers.d
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
// This file contains mixin templates that define the parse time fields and methods of AST nodes.
module dmd.astmembers;

mixin template parseTimePropertiesAliasThis()
{
Identifier ident;

extern (D) this(const ref Loc loc, Identifier ident)
{
super(loc, null); // it's anonymous (no identifier)
this.ident = ident;
}

override void accept(Visitor v)
{
v.visit(this);
}
}