Skip to content
This repository has been archived by the owner on Sep 20, 2021. It is now read-only.

Fix the incorrect display of the column and line in errors. #79

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

SerafimArts
Copy link
Contributor

@SerafimArts SerafimArts commented Nov 1, 2017

Fixes "Incorrect calculation of line and position in utf-8 files" issue

Unrecognized token:

image

Unexpected token:

image

😸

@SerafimArts SerafimArts changed the title Incorrect calculation of line and position in utf-8 files Fix the incorrect display of the column and line in errors. Nov 1, 2017
@Hywan Hywan self-assigned this Nov 1, 2017
@Hywan
Copy link
Member

Hywan commented Nov 1, 2017

Thanks for the PR!

I would use a custom exception constructor (that can use a trait or a parent class) to compute the correct line, column, marker/arrow position etc. rather than a trait in the Lexer and the Parser classes. It makes more sense for me.

Can you update your patch this way please? If you don't have time, I can do it for you.

@SerafimArts
Copy link
Contributor Author

SerafimArts commented Nov 1, 2017

@Hywan With such a signature?

$error = SomeErr::createFromOffest(string $message, array $args = [], string $sources, int $offset)

@SerafimArts
Copy link
Contributor Author

Or maybe:

$message = \sprintf('...', ...$args);

throw SomeErr::fromOffset($message, string $sources, int $offset);

@Hywan
Copy link
Member

Hywan commented Nov 6, 2017

What about:

class UnrecognizedToken extends Exception
{
    …

    public function __construct($code, $text, $byteOffset)
    {
        // …
        parent::__construct($message, $code, $args);
    }
}

Notice that $message and $args are no longer part of the signature, but they are computed and passed to the parent constructor.

Thoughts?

@SerafimArts
Copy link
Contributor Author

This will make it impossible to reuse the exception with arguments that are different from those specified in the constructor. And this is a violation of backward compatibility.

Is such an improvement of such changes worthwhile?

@Hywan
Copy link
Member

Hywan commented Nov 6, 2017

Or add a static method on UnrecognizedToken to build a valid UnrecognizedToken exception with a pre-defined message? Something like:

class UnrecognizedToken extends Exception
{
    public static function blabla(…)
    {
        // $message
        return new self($message, …);
    }
}

@SerafimArts
Copy link
Contributor Author

This I suggested above. Just now there is no time to realize =\

I think I'll fix it this week.

@Hywan
Copy link
Member

Hywan commented Nov 7, 2017

Heh, sorry :-).

@SerafimArts SerafimArts mentioned this pull request Nov 10, 2017
25 tasks
@SerafimArts
Copy link
Contributor Author

@Hywan ping?

@SerafimArts SerafimArts deleted the issue/78 branch January 10, 2018 06:56
@ghost ghost removed the in progress label Jan 10, 2018
@Pierozi
Copy link
Member

Pierozi commented Jan 10, 2018

@SerafimArts PR was closed for a reason?

@SerafimArts
Copy link
Contributor Author

SerafimArts commented Jan 11, 2018

@Pierozi for me it became not relevant. I had to fork the project and slightly rewrite it: https://github.com/railt/compiler/network

@Hywan
Copy link
Member

Hywan commented Jan 11, 2018

We are still interested by the PR though… Can you reopen it please?

@SerafimArts
Copy link
Contributor Author

nope =(
image

@Pierozi
Copy link
Member

Pierozi commented Jan 11, 2018

you can still get the code of the PR, in order to recreate your branch

$ git clone [email protected]:hoaproject/Compiler.git
$ cd Compiler
$ git fetch origin pull/79/head:issue/78
$ git remote add my-fork ....
$ git push my-fork issue/78

@SerafimArts SerafimArts reopened this Jan 11, 2018
@SerafimArts SerafimArts restored the issue/78 branch January 11, 2018 09:26
@SerafimArts
Copy link
Contributor Author

Done

@SerafimArts
Copy link
Contributor Author

In any case, in this PR I did not take into account the behavior of %pragma lexer.unicode, so it is not entirely correct. More precisely, exceptions always take utf into account, regardless of whether there are instructions to use it.

@Hywan
Copy link
Member

Hywan commented Jan 22, 2018

Thanks for reopening!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Development

Successfully merging this pull request may close these issues.

3 participants