dlang-community/SDLang-D

Access Violation Pull Parser

marler8997 opened this issue · 3 comments

I found an issue with the pullParseSource function. It appears that your InputVisitor class saves a pointer to the object passed to the constructor. The problem is that since you are allocating the PullParser object on the stack here:

auto pullParseSource(string source, string filename=null)
{
    auto lexer = new Lexer(source, filename);
    auto parser = PullParser(lexer); // STACK allocation
    return inputVisitor!ParserEvent( *parser );
}

Once the pullParseSource function returns the PullParser object has been deallocated. This is currently causing an AccessViolation as soon as I start using the PullParser after calling this function.

You could solve this by allocating the PullParser object on the heap but that doesn't give the developer the ability to be able to specify whether they want a stack or heap allocated PullParser. I've provided some thoughts/solutions to this problem.

You can enhance your InputVisitor class to be able to save an instance to the object. This does not mean that the InputVisitor class does not support saving a pointer to the object as well, it just means that you give the InputVisitor class the ability to use either a pointer to the object or an instance of it. I'm not sure exactly how you would want to support this (an extra template parameter or something?) so I'll let you figure that out.

Another thought: It would probably be a good idea to create another pullParse function, something like this:

auto pullParse(scope Lexer lexer)
{
    auto parser = PullParser(lexer);
    return inputVisitor!ParserEvent( parser ); // NOTE: you would still need to fix inputVisitor to save an instance to PullParser instead of a reference to it
}

This way, a developer can allocate they Lexer class on the stack like this:

scope lexer = new Lexer(sdlText, filename); // Ensures lexer allocated on the stack
foreach(event; pullParse(lexer)) {
    //...
}

FWIW, The line is pullParseSource is actually: return inputVisitor!ParserEvent( parser );

However you're right, inputVisitor is clearly saving and using a pointer to a stack-allocated object. I don't know how that managed to work for me in the first place.

There was a reason I changed inputVisitor to take a reference instead of a value, but I'll have to try to remember what the heck the reason was.

Also, I didn't think "scope" was currently functional...?

Oh I didn't know scope wasn't functional. I just discovered it recently. I think there are some cases you would want input visitor to save a reference and some cases you would want it to save an instance. If you could just allow the user to specify which one in the template parameter I think that would be best.

Oh I didn't know scope wasn't functional.

I'm actually not really sure either way :/ That's one I keep looking track of just what exactly's up with it.