Skip to content

Conversation

Thomasb81
Copy link
Contributor

Hello

I was looking to complete the default error message with the file name.

But with previous syntaxError prototype:

 virtual void syntaxError(IRecognizer *recognizer, Token *offendingSymbol, size_t line,
                             size_t charPositionInLine, const std::string &msg, std::exception_ptr e) = 0;

It was not possible because IRecognizer does not have appropriate helper. While the above method documentation state that parameter recognizer allow to :

    ///<param name="recognizer">
    ///        What parser got the error. From this
    /// 		  object, you can access the context as well
    /// 		  as the input stream. </param>

So I change the type of *recognizer from IRecognizer to Reconizer :

 virtual void syntaxError(Recognizer *recognizer, Token *offendingSymbol, size_t line,
                             size_t charPositionInLine, const std::string &msg, std::exception_ptr e) = 0;

After this fix it is now possible to get source filename:

	void syntaxError(
			Recognizer *recognizer,
			Token *offendingSymbol,
			size_t line,
			size_t charPositionInLine,
			const std::string &msg,
			std::exception_ptr e){
	    std::cerr << recognizer->getInputStream()->getSourceName()<< line << ":" << charPositionInLine << ":Error:" << msg << std::endl;
	};

Thomasb81 added a commit to Thomasb81/hdlConvertor that referenced this pull request Feb 27, 2017
@mike-lischke
Copy link
Member

I believe we should rather add methods to the interface if we need some. Even though I don't see why an interface is a good choice here, it is now there and we should stay with it, to avoid deviating too much from other targets.

@Thomasb81
Copy link
Contributor Author

Ok fine.

Before proposing this fix I had a quick look at the java runtime, and did not find a class name IRecognizer but Recognizer and it works. So I bet on a typo that has not been seen up to now...

I understand that I probably misunderstand how it was suppose to work. Now I do not fully understand how you wish to resolve this issue.

My proposed fix do not match you expectation should I close the push request or should I let it open and someone else will revert it and propose a better fix ?
Thomas

@parrt
Copy link
Member

parrt commented Mar 1, 2017

@mike-lischke should we alter this one or try to open a new one?

@Thomasb81
Copy link
Contributor Author

Could it be a solution to remove this interface class IReconizer and add an alias :
using IReconizer = Reconizer;
I don't know it this is legal C++. Then deprecate the IReconizer class
So compatibility is preserved for the moment , and user will see a compilation warning...
Later we can remove the alias.
Is this a valid possible solution ?

Thomas

@mike-lischke
Copy link
Member

I'm sorry for the confusion here. My fault, I answered without looking up the code again. That IRecognizer class came from the automatic translation tool that was used at the beginning to get the initial code for the C++ target. There is actually no need to have that interface there. So the simplest solution would be to remove that interface and rename all IRecognizer references to just Recognizer. Can you do that @Thomasb81?

@mike-lischke
Copy link
Member

This will likely need an update to remove the IRecognizer.h file from the XCode project, but that should be done using XCode, not just by doing it in the project source code.

@Thomasb81
Copy link
Contributor Author

The dedicated commit related to xcode can be revert but I do not have access to xcode.

@mike-lischke
Copy link
Member

Yes, prelease revert that part. I'll take care for XCode. Might be you hit already all places, but better let XCode do the manipulation.

@parrt parrt added this to the 4.7 milestone Mar 3, 2017
@parrt
Copy link
Member

parrt commented Mar 3, 2017

Actually does this fix #1708 ?

@parrt parrt merged commit 4a5e65c into antlr:master Mar 3, 2017
@Thomasb81
Copy link
Contributor Author

Thomasb81 commented Mar 3, 2017

No
Basically #1708 the test case trig a NoViableAltException while it should not : the parsed input was ok on tagged version 4.6.

So completely different except both are related to error. :)

@parrt
Copy link
Member

parrt commented Mar 3, 2017

ah! Thanks.

@Thomasb81 Thomasb81 deleted the Fix_syntaxError_proto branch March 4, 2017 10:48
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.

3 participants