Skip to content

extract mangleExpression() from Mangler #14471

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

Jump to bottom
Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

extract mangleExpression() from Mangler #14471

wants to merge 1 commit into from

Conversation

WalterBright
Copy link
Member

Use nested functions instead of visitor.

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @WalterBright!

Bugzilla references

Your PR doesn't reference any Bugzilla issue.

If your PR contains non-trivial changes, please reference a Bugzilla issue or create a manual changelog.

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub run digger -- build "master + dmd#14471"

@WalterBright
Copy link
Member Author

The buildkite issues appear to be heisenbugs.

@WalterBright
Copy link
Member Author

@RazvanN7 Ready to go.

RazvanN7
RazvanN7 requested changes Oct 11, 2022
View reviewed changes
Copy link
Contributor

@RazvanN7 RazvanN7 left a comment

Choose a reason for hiding this comment

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

I don't understand why the interface was changed to receive Backref instead of a type. It is much more elegant to instantiate the Backref at the callee site rather than uselessly sending a reference from the called.

compiler/src/dmd/dmangle.d
this.buf = buf;
this.backref = Backref(rootType);
}

void mangleSymbol(Dsymbol s)
Copy link
Contributor

Choose a reason for hiding this comment

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

This and the following functions should be indented properly.

compiler/src/dmd/dmangle.d
assert(0);
}

}
Copy link
Contributor

Choose a reason for hiding this comment

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

Please mind the indentation.

compiler/src/dmd/dmangle.d
if (t.deco)
buf.writestring(t.deco);
else
{
scope Mangler v = new Mangler(buf, t);
v.visitWithMask(t, 0);
auto backref = Backref(t);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this kept outisde of the mangleType function? It doesn't really make any sense to put this burden on the caller. Just have mangleType receive a type as the class constructor did and instantiate the backref there.

compiler/src/dmd/dmangle.d

/*************************************************************
*/
void mangleFuncType(TypeFunction t, TypeFunction ta, ubyte modMask, Type tret, OutBuffer* buf, ref Backref backref)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be private.

compiler/src/dmd/dmangle.d

/*************************************************************
*/
void mangleParameter(Parameter p, OutBuffer* buf, ref Backref backref)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be private.

compiler/src/dmd/dmangle.d
if (!fd.mangleString)
{
OutBuffer buf;
scope Mangler v = new Mangler(&buf);
auto backref = Backref(null);
Copy link
Contributor

Choose a reason for hiding this comment

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

Why is this outside of the Mangler constructor?

compiler/src/dmd/dmangle.d
* buf = buffer to append mangling to
* backref = state of back references (updated)
*/
void mangleType(Type t, ubyte modMask, OutBuffer* buf, ref Backref backref)
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be private.

@dkorpel
Copy link
Contributor

dkorpel commented Oct 27, 2022

@WalterBright Please address Razvan's review

ibuclaw
ibuclaw requested changes Oct 27, 2022
View reviewed changes
compiler/src/dmd/dmangle.d
Comment on lines +366 to +416
case Tvoid:
case Tint8:
case Tuns8:
case Tint16:
case Tuns16:
case Tint32:
case Tuns32:
case Tint64:

case Tuns64:
case Tfloat32:
case Tfloat64:
case Tfloat80:
case Timaginary32:
case Timaginary64:
case Timaginary80:
case Tcomplex32:
case Tcomplex64:
case Tcomplex80:

case Tbool:
case Tchar:
case Twchar:
case Tdchar:

case Tint128:
case Tuns128: visitType(t); break;

case Tpointer:
case Treference:
case Tdelegate:
case Tslice: visitTypeNext (cast(TypeNext)t); break;

case Tarray: visitTypeDArray (t.isTypeDArray()); break;
case Tsarray: visitTypeSArray (t.isTypeSArray()); break;
case Taarray: visitTypeAArray (t.isTypeAArray()); break;
case Tfunction: visitTypeFunction (t.isTypeFunction()); break;
case Tident: visitTypeIdentifier(t.isTypeIdentifier()); break;
case Tclass: visitTypeClass (t.isTypeClass()); break;
case Tstruct: visitTypeStruct (t.isTypeStruct()); break;
case Tenum: visitTypeEnum (t.isTypeEnum()); break;
case Ttuple: visitTypeTuple (t.isTypeTuple()); break;
case Tnull: visitTypeNull (t.isTypeNull()); break;
case Tvector: visitTypeVector (t.isTypeVector()); break;
case Tnoreturn: visitTypeNoreturn (t.isTypeNoreturn); break;

case Terror:
break; // ignore errors

default:
assert(0);
Copy link
Member

@ibuclaw ibuclaw Oct 27, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

This was already reverted because it caused a regression.

#14477

Copy link
Member Author

Choose a reason for hiding this comment

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

It would help if you said what the regression was.

Copy link
Member

Choose a reason for hiding this comment

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

I did in the introducing pr
#14468 (comment)

@RazvanN7
Copy link
Contributor

RazvanN7 commented Nov 1, 2022

ping @WalterBright

@RazvanN7
Copy link
Contributor

@WalterBright are you still working on this?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ibuclaw ibuclaw ibuclaw requested changes

@RazvanN7 RazvanN7 RazvanN7 requested changes

Assignees
No one assigned
Labels
Merge:needs rebase Review:Easy Review Review:Needs Rebase Review:Needs Work Review:stalled Severity:Refactoring No semantic changes to code stalled
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@WalterBright @dlang-bot @dkorpel @RazvanN7 @ibuclaw