Skip to content

Fix regression where function overload candidates might not be reported #843

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

Closed

Conversation

marsupial
Copy link
Contributor

Description

597b6b1 introduced a bug which might make the error message less verbose than it used to be.
This rectifies that.

Tests

oslc-err-funcoverload

Checklist:

  • I have read the contribution guidelines.
  • I have previously submitted a Contributor License Agreement.
  • I have updated the documentation, if applicable.
  • I have ensured that the change is tested somewhere in the testsuite (adding new test cases if necessary).
  • My code follows the prevailing code style of this project.

@marsupial marsupial mentioned this pull request Jan 20, 2018
5 tasks
@lgritz
Copy link
Collaborator

lgritz commented Jan 20, 2018

Something is wrong, it's failing the CI tests, no longer correctly compiling some of the MaterialX shaders.

@marsupial
Copy link
Contributor Author

Yes, was not using the proper constant.
Running on a separate CI before updating the PR.

@marsupial
Copy link
Contributor Author

Should be good to go. Was actually trying to fix some other issues that are coming to light, but just reverted to simple fix for missing labels of some ambiguities.

@marsupial
Copy link
Contributor Author

Might be prudent to revert #837 (597b6b1).
Not sure how it got missed, but I'm seeing some behavior that is definitely incorrect.

@lgritz
Copy link
Collaborator

lgritz commented Jan 20, 2018

So, just to clarify:

You think we should revert #837, but keep this one #843?

@lgritz
Copy link
Collaborator

lgritz commented Jan 20, 2018

Or did you mean we should put both on hold until you sort it out?

@marsupial
Copy link
Contributor Author

I was thinking reverting #837 and rolling this (#843) into either a new PR or re-open #837.
If you're more interested in not reverting and moving forward with fixing the original problem I'll have a few more hours today and tomorrow (which should be enough to nail it down), but a back and forth might be necessary.

I'll do a further writeup of the problems I'm discovering in the next hour.

@lgritz
Copy link
Collaborator

lgritz commented Jan 20, 2018

OK, since we know there's something wrong, I'll revert #837 in master, and you can close this PR, and then when you have it totally worked out, just post a new combined PR (or re-open 837). There's no rush on this, we've obviously lived with the existing ambiguous edge cases for a long time with nobody complaining.

@marsupial marsupial closed this Jan 21, 2018
@marsupial marsupial deleted the funcregression branch February 2, 2018 16:52
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.

2 participants