[DDMD] [refactor] Move Expression::interpret into a visitor class

[yebblies]

Haha, diff too big to be shown.

[ghost]

Is this comment still relevant or can you remove it? https://github.com/yebblies/dmd/blob/expinterpret/src/interpret.c#L2167

[ghost]

Just one thing, there's apparently a performance regression since recently: https://d.puremagic.com/issues/show_bug.cgi?id=12137

I don't want to point fingers since I haven't done any tests, but have you noticed whether these pulls have made any impact on performance?

[ghost]

Nitpick:

assert(((IndexExp*)el)->e1 != e);

el and e1 are hard to distinguish. In Github's font they're almost impossible to distinguish.

[ghost]

s/e->op==TOKadd)/e->op == TOKadd since you're already refactoring.

[ghost]

A few things I really like about these transformations:

1. All related code is in one place, rather than being scattered around virtual functions. In D it wouldn't even be possible to move the virtual methods into a single module.
2. No implicit this when accessing fields. This means all side-effects for an object are clearly visible because you can tell at a glance whether a manipulated variable is actually a field or a stack variable. There are only a few members the visitors usually declare (e.g. result). Personally I'd even use this->result but the current code isn't bad either.

[ghost]

And #3:

Rather than having to chase recursive calls (and see where a result is finally returned), you can immediately tell where a result is stored. return new IntegerExp(loc, cmp, type); doesn't mean much since you don't know which function called this function at a glance, but with result = new IntegerExp(e->loc, cmp, e->type); you know where the value is heading to. It makes tracking function calls and state changes (and generally understanding the code flow) more linear, imo.

[ghost]

Here's a classic example of what I said in #2 which I just found in the old diff:

Expression *e1 = this->e1;

That's just horrible code, hiding a this field behind a stack variable like that.

[ghost]

s/e->e1->op==TOKvar/e->e1->op == TOKvar please, thanks. :)

[yebblies]

Is this comment still relevant or can you remove it?

I don't know what you mean, that line is not a comment.

I don't want to point fingers since I haven't done any tests, but have you noticed whether these pulls have made any impact on performance?

I honestly have no idea, but I didn't introduce any new heap allocations. Kenji's comment makes it sound like a problem with the beta build.

el and e1 are hard to distinguish. In Github's font they're almost impossible to distinguish.

Changed to exp

s/e->op==TOKadd)/e->op == TOKadd since you're already refactoring.

Ok, spaced out all the ==. Note that the D conversion tool will do this automatically.

A few things I really like about these transformations:

My main gripe with it is that all returns become fugly. Things like toCBuffer work wonderfully but this and optimize are painful.

I'm not so sure about 3, you can make the returns very hard to follow with a couple of helpers. It is nice though to have a single entry point in case you want to check the result.

[ghost]

Around: https://github.com/yebblies/dmd/blob/expinterpret/src/interpret.c#L3564

e1 = new SliceExp(e->loc, e1,
    new IntegerExp(e->loc, 0, Type::tsize_t),

This used to be:

e1 = new SliceExp(loc, e1,
    new IntegerExp(Loc(), 0, Type::tsize_t),

IOW, a default Loc() was used for the IntegerExp before the pull. Was the change deliberate? It might change diagnostics (which likely aren't even tested since the pull is passing the autotester).

[yebblies]

Expression *e1 = this->e1;

Yeah that's pretty nasty.

[ghost]

I don't know what you mean, that line is not a comment.

Damn github is wrongly displaying the line count for me, I meant this comment:

/* In both D1 and D2, attempts to modify string literals are prevented
* in BinExp::interpretAssignCommon.
* In D2, we also disallow casts of read-only literals to mutable,
* though it isn't strictly necessary.
*/

[yebblies]

This used to be:

Are you sure? That seems highly unlikely, is it possible it's a diff mismatch?

[yebblies]

It might change diagnostics (which likely aren't even tested since the pull is passing the autotester).

If it did it would change no-line-number into has-line-number which would be an improvement,

[ghost]

Yeah those return; statements are a little ugly. :>

[ghost]

Are you sure? That seems highly unlikely, is it possible it's a diff mismatch?

[ghost]

Well if it improves the diagnostics all the better, but I'm not sure why Loc() was used in the first place tbh.

[yebblies]

The comment was lying, I changed it to:

        /* Attempts to modify string literals are prevented
         * in BinExp::interpretAssignCommon.
         */

[ghost]

I'm gonna have to start pasting image diffs since I can't properly link to the right line numbers.

Here you're assigning oldval instead of newval to result:

[yebblies]

Well if it improves the diagnostics all the better, but I'm not sure why Loc() was used in the first place tbh.

Well that certainly does look like I changed it. I still have no idea how that could have happened though.

It would have used Loc() because there was ~0 chance of it every being in an error message.

[yebblies]

Here you're assigning oldval instead of newval to result:

Oops. Well I know how that one happened, fixed.

[ghost]

This would have to go into one of Steve McConnell's books for how not to write APIs:

if (e->isBool(false))
    result = 0;
else if (isTrueBool(e))
    result = 1;

[yebblies]

I never really grasped how enormous the interpreter was before this pull. 10000 lines of crazy special cases.

[ghost]

Review done.

[ghost]

Auto-merge toggled on

[ghost]

Thanks for the hard work.

[yebblies]

Thanks for the hard work.

Thanks for the hard review. These pulls would be going nowhere without your help. I better get some sleep now, it's almost 6am here.

[yebblies]

Win32 failure is not pull-related.