Fix #2727: improve setTokenFactory in Cpp target #2733
Conversation
runtime/Cpp/runtime/src/Lexer.h
Outdated
| this->_factory = factory; | ||
| template<typename Factory> | ||
| void setTokenFactory(Ref<Factory> const& factory) { | ||
| this->_factory = std::static_pointer_cast<TokenFactory<CommonToken>>(factory); |
There was a problem hiding this comment.
I'm afraid this is the wrong approach. Look at the Java source code, which uses public void setTokenFactory(TokenFactory<?> factory), so it's clear the token class must be templated, not the token factory class. And your cast wouldn't work if something else but a CommonTokenFactory is passed in.
This part of the C++ runtime is a bit tricky, as there's no simple equivalent for the Java generic used for it (C++ cannot directly define templated member variables). The correct approach to use here is a simple pointer. This is also what is used in the other classes (e.g. see setInterpreter and addErrorListener in Recognizer.h, which all just use a plain object pointer). The caller of setTokenFactory is reponsible to manage the memory used by the factory instance. This works also well with the factory function for token factories (see TokenFactory::create, which creates a unique pointer).
The default factory also should be declared with a unique pointer.
There was a problem hiding this comment.
Great, thanks for this guidance. There were a number of ways to go here; I went with one that preserved the current type of the Lexer::_factory member. Your feedback clarifies that changing the type of that member is ok, and I can see the rest will flow from that. I'll post a revision here shortly.
Change the setTokenFactory template functions and the getTokenFactory functions to use a plain pointer rather than a Ref. This makes the caller of setTokenFactory responsible for managing the lifetime and memory of the token factory instance they pass in. Change CommonTokenFactory::DEFAULT to be a unique_ptr, and correct all places using it as a Ref.
vinoski
force-pushed
the
cpp-set-token-factory
branch
from
64a4747 to
38200b6
Compare
Change the setTokenFactory template function in the Lexer, Parser, and
TokenSource classes to be parameterized by Factory type rather than by
token type. Change the setTokenFactory signature to require the
function argument to be a Ref of the Factory template parameter type
so it can be correctly converted and assigned to the token factory
member. The type of that member is Ref<TokenFactory>,
which implies that the Factory template parameter must be of a type
convertible to TokenFactory.