Skip to content

Make the Visitor class inherit the ParseTimeVisitor#7438

Merged
dlang-bot merged 1 commit intodlang:masterfrom
RazvanN7:Visitor_class
Dec 18, 2017
Merged

Make the Visitor class inherit the ParseTimeVisitor#7438
dlang-bot merged 1 commit intodlang:masterfrom
RazvanN7:Visitor_class

Conversation

@RazvanN7
Copy link
Contributor

@ibuclaw Is there anything else that should be done in order to make the visitor compatible with gdc? Should the header files be updated so that the inheritance is visible?

@dlang-bot
Copy link
Contributor

Thanks for your pull request, @RazvanN7! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.

Some tips to help speed things up:

  • smaller, focused PRs are easier to review than big ones

  • try not to mix up refactoring or style changes with bug fixes or feature enhancements

  • provide helpful commit messages explaining the rationale behind each change

Bear in mind that large or tricky changes may require multiple rounds of review and revision.

Please see CONTRIBUTING.md for more information.

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.

@andralex
Copy link
Member

Eh, that's a fair amount of duplication, but I guess it'll do. @RazvanN7 what's with the unittest failures? @ibuclaw does this float your boat?

@RazvanN7
Copy link
Contributor Author

RazvanN7 commented Dec 15, 2017

@andralex I don't know what duplication you are talking about.
Those must have been spurious failures since everything seems to be green now.

Later edit: It seems that travis fails a build on a betterC test, altough I don't think it's related to this PR

@PetarKirov
Copy link
Member

Also CC @kinke @JohanEngelen and @klickverbot.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 15, 2017

I think that we should hold this off until some tests are in place. Otherwise you're just shooting in the dark here.

@RazvanN7
Copy link
Contributor Author

@ibuclaw Indeed, but the last PR made it so that the Visitor is just an alias to a templated class. This PR makes the visitor an extern C++ class again so it can't make things worse.

@andralex
Copy link
Member

@ibuclaw I'll merge this under the assumption it'll make things better even before having added tests.

Copy link
Member

@ibuclaw ibuclaw left a comment

Choose a reason for hiding this comment

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

@andralex @RazvanN7 - OK I can confirm 👍 on both accounts.

First: I was correct in assuming that this broke C++.
Second: This patch fixes it.

@ibuclaw
Copy link
Member

ibuclaw commented Dec 18, 2017

Here's a small test:

#include "expression.h"
#include "globals.h"
#include "module.h"
#include "mtype.h"
#include "objc.h"
#include "target.h"
#include "visitor.h"

/**********************************/

extern "C" int rt_init();
extern "C" void gc_disable();

void initialize()
{
    rt_init();
    gc_disable();

    global._init();
    global.params.isLinux = true;

    Type::_init();
    Module::_init();
    Expression::_init();
    Objc::_init();
    Target::_init();
}

/**********************************/

class TestVisitor : public Visitor
{
  public:
    void visit(Expression *)
    {
        printf("Success!\n");
    }
};

void test_visitors()
{
    TestVisitor tv;
    IntegerExp *ie = IntegerExp::create(Loc(), 42, Type::tint32);
    ie->accept(&tv);
}

/**********************************/

int main(int argc, char **argv)
{
    initialize();

    test_visitors();
}

@ibuclaw
Copy link
Member

ibuclaw commented Dec 18, 2017

Without this patch:

$ ../generated/linux/release/64/dmd-cxx
dmd-cxx: dmd/visitor.h:374: virtual void Visitor::visit(Dsymbol*): Assertion `0' failed.
Aborted

Backtrace:

#0  0x00007ffff6d75428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
#1  0x00007ffff6d7702a in __GI_abort () at abort.c:89
#2  0x00007ffff6d6dbd7 in __assert_fail_base (fmt=<optimized out>, assertion=assertion@entry=0x79410e "0", file=file@entry=0x794100 "dmd/
visitor.h", line=line@entry=374, function=function@entry=0x794220 <Visitor::visit(Dsymbol*)::__PRETTY_FUNCTION__> "virtual void Visitor::
visit(Dsymbol*)") at assert.c:92
#3  0x00007ffff6d6dc82 in __GI___assert_fail (assertion=0x79410e "0", file=0x794100 "dmd/visitor.h", line=374, function=0x794220 <Visitor
::visit(Dsymbol*)::__PRETTY_FUNCTION__> "virtual void Visitor::visit(Dsymbol*)") at assert.c:101
#4  0x00000000006a806b in Visitor::visit (Dsymbol *) at dmd/visitor.h:374
#5  0x00000000006a8820 in Visitor::visit (Declaration *) at dmd/visitor.h:417
#6  0x00000000006a88f0 in Visitor::visit (VarDeclaration *) at dmd/visitor.h:421
#7  0x00000000006a898c in Visitor::visit (TypeInfoDeclaration *) at dmd/visitor.h:425
#8  0x00000000006a8a90 in Visitor::visit (TypeInfoArrayDeclaration *) at dmd/visitor.h:430
#9  0x000000000059cf1a in IntegerExp::accept(GenericVisitor<ASTCodegen>*) (this=0x7ffff7ed51e0, v=0x7fffffffdbd0) at dmd/expression.d:254
#10 0x00000000006a7330 in test_visitors () at dmd/cxxtest.c:61

With this patch:

$ ../generated/linux/release/64/dmd-cxx
Success!

@ibuclaw
Copy link
Member

ibuclaw commented Dec 18, 2017

@MartinNowak - We are under freeze for release now right? If applicable, this patch may need to be applied to stable.

@dlang-bot dlang-bot merged commit 2316ebb into dlang:master Dec 18, 2017
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.

5 participants