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

ion_catalog_find_symbol_table doesn't find anything #267

Merged
merged 1 commit into from
Dec 21, 2021

Conversation

m6w6
Copy link
Contributor

@m6w6 m6w6 commented Dec 17, 2021

Description of changes:
ion_catalog_find_symbol_table() doesn't find anything because of not using pointers correctly with the ION_COLLECTION API.

Just compare the code of _ion_catalog_find_symbol_table_helper() with _ion_catalog_find_best_match_helper().

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.

@cheqianh
Copy link
Contributor

cheqianh commented Dec 17, 2021

Thanks for the contribution!

It seems like some tests failed. Took a brief look, e.g. this test failed due to line 265.

I'll try to do a little more investigation later.

@m6w6 m6w6 force-pushed the ion_catalog_find_symbol_table branch 2 times, most recently from 1cb8b02 to b0bcc6f Compare December 21, 2021 09:12
m6w6 added a commit to awesomized/ext-ion that referenced this pull request Dec 21, 2021
@m6w6
Copy link
Contributor Author

m6w6 commented Dec 21, 2021

Sorry, the first patch was broken, too, though accidentally working for me in a quick test at first.

Looks better now.

@m6w6 m6w6 force-pushed the ion_catalog_find_symbol_table branch from b0bcc6f to 60fa439 Compare December 21, 2021 09:19
Copy link
Contributor

@cheqianh cheqianh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@cheqianh cheqianh merged commit 9310a99 into amazon-ion:master Dec 21, 2021
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