-
-
Notifications
You must be signed in to change notification settings - Fork 12
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
Cpp executor - first collation conformance tests with ICU4C #106
Conversation
I think this needs to explicitly reference the particular JSON library. Running ./executor gives with any input gives: |
Yikes, some of my Schema stuff has leaked into this. |
executors/cpp/Makefile
Outdated
# export ICU4C= $location | ||
# export LD_LIBRARY_PATH=$ICU4C/lib:$LD_LIBRARY_PATH | ||
# export PATH=$ICU4C/bin:$PATH | ||
# export CPATH=$ICU4C/bin:$CPATH | ||
# - do 'make' in this directory | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't see json-c mentioned anywhere; seems like it should be?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If it's installed properly, then an explicit reference isn't needed.
Updated this PR with ICU4C collation versions 71, 72, 73, and 74. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you pull out the ICU4C number formatting code, it looks good to me. Save any rework you need to do on ICU4C number formatting executor code for a separate PR. I can help you with git stuff to split your code out into separate branches accordingly if that's what you want to do.
Many of the comments that you replied to don't have the changes applied in the PR. Did you make the commits locally without pushing them? Check Sublime Merge to visually see what's going on. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
|
||
// What was the actual returned value? | ||
json_object_object_add( | ||
return_json, "compare", json_object_new_int64(numeric_result)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yay, looking good (by using numeric_result
as the arg to json_object_new_int64
)
executors/test_strings.txt
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for updating this file accordingly.
This adds files for compiling and executing C++ with ICU4C in conformance testing. This is not yet ready for integration.