Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix the compiler suggesting unimplemented variables #374

Merged
merged 2 commits into from
Dec 31, 2018

Conversation

Daniel-Cortez
Copy link
Contributor

@Daniel-Cortez Daniel-Cortez commented Oct 3, 2018

What this PR does / why we need it:

This PR fixes the problem where after the 1'st pass the compiler suggests global variables before the point of their definition:

GetValue()
{
    return value; // error 017: undefined symbol "value"; did you mean "value"?
}

new value = 0;

main()
{
    GetValue();
}

Which issue(s) this PR fixes:

What kind of pull this is:

  • A Bug Fix
  • A New Feature
  • Some repository meta (documentation, etc)
  • Other

Additional Documentation:

@Daniel-Cortez Daniel-Cortez requested a review from a team as a code owner October 3, 2018 13:37
@YashasSamaga
Copy link
Member

YashasSamaga commented Dec 30, 2018

not related specifically to this PR but to the system

There is an assert(0) in error_suggest but doing tagof(X) where X is a string, number, etc. leads to that assertion because of error_suggest(20,st,NULL,estNONSYMBOL,tok);. Since tok is passed as subtype, it can be tRATIONAL, tNUMBER, tSTRING, etc. depending on what the user entered.

The previous version of the compiler was giving out errors instead of an assertion failure.

@Y-Less
Copy link
Member

Y-Less commented Dec 30, 2018

I have wished you could take the tag of literals a few times:

tagof (X:5)

Not obviously useful as typed, but is in macro outputs. But I have macros to remove anything after the : for now.

@Daniel-Cortez
Copy link
Contributor Author

Ok, I fixed this and a couple of other bugs locally. Would it be alright if I turn this PR into a more general one and upload those bugfixes here? (Just asking because this PR has already been approved in the current state.)

@Y-Less
Copy link
Member

Y-Less commented Dec 30, 2018

No, make those a separate pull request. The point of approvals is to show that the code's been checked, adding more after that defeats the purpose.

@Daniel-Cortez
Copy link
Contributor Author

Daniel-Cortez commented Dec 31, 2018

Well, I wasn't asking this only because this PR has already been approved, but also because uploading those fixes in a separate PR would definitely lead to a mess with merge conflicts, but ok.

@YashasSamaga
Copy link
Member

YashasSamaga commented Dec 31, 2018

I think every new feature should have a corresponding issue. I couldn't find an issue for this system which is why I sneaked it in here. I could have made an issue instead of commenting it here though.

@YashasSamaga YashasSamaga merged commit db7efa7 into pawn-lang:dev Dec 31, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants