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

Unicode charset #504

Merged
merged 1 commit into from May 7, 2019
Merged

Unicode charset #504

merged 1 commit into from May 7, 2019

Conversation

cmazakas
Copy link
Member

@cmazakas cmazakas commented May 4, 2019

This pull request gives X3 Unicode literal support for its char_set<boost::spirit::char_encoding::unicode>. This passively enables boost::spirit::x3::unicode::char_ to now support string literals such as: U"hello, \u20ac!".

This PR also extends the X3 test utility to work with boost::basic_string_view as well.

@djowel
Copy link
Member

djowel commented May 4, 2019

Looks good. I'll study this as soon as I fix the (unrelated) failing tests on Visual Studio.

@djowel
Copy link
Member

djowel commented May 4, 2019

One comment ^^, otherwise it looks good. I just fixed the (unrelated) VS failing test.

@djowel djowel self-assigned this May 4, 2019
@djowel djowel requested a review from Kojoley May 4, 2019 19:32
@@ -0,0 +1,89 @@
#define BOOST_SPIRIT_X3_UNICODE
Copy link
Member

Choose a reason for hiding this comment

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

You'll need a copyright notice at the top with the Boost license. See other files.

Copy link
Member Author

Choose a reason for hiding this comment

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

Added!

@@ -93,42 +93,63 @@ namespace boost { namespace spirit { namespace x3 { namespace traits
template <>
struct char_type_of<wchar_t> : mpl::identity<wchar_t> {};

template <>
struct char_type_of<char32_t> : mpl::identity<char32_t> {};
Copy link
Collaborator

@Kojoley Kojoley May 5, 2019

Choose a reason for hiding this comment

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

I am inclined towards simplifying and generalizing the char_type_of instead because there are more char types and soon or later the question will rise again.

Copy link
Member

Choose a reason for hiding this comment

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

I am inclined towards simplifying and generalizing the char_type_of instead because there are more char types and soon or later the question will rise again.

Makes sense. I'd love to see some ideas in code.

Copy link
Collaborator

Choose a reason for hiding this comment

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

See #507

Copy link
Member Author

@cmazakas cmazakas May 6, 2019

Choose a reason for hiding this comment

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

Oh cool, that PR obsoletes mine then, no? I'm fine with that. Less code maintenance in general is better.

We can still keep the small tests that I added though as those still have some value.

Copy link
Member

Choose a reason for hiding this comment

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

We can still keep the small tests that I added though as those still have some value.

Absolutely. I just merged #507. Could you please do a final tweak on this PR to deal the changes? Thanks!

Copy link
Member Author

Choose a reason for hiding this comment

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

Alright, took a big of git magic to get there but I managed to condense my PR down to 1 commit that just updates the test files.

// ascii
{
using namespace boost::spirit::x3;
using char_set = char_set<boost::spirit::char_encoding::ascii>;
Copy link
Collaborator

@Kojoley Kojoley May 5, 2019

Choose a reason for hiding this comment

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

Ain't we already have those tests?

char_set is just an underlying parser of char_("bla") if you did not know it https://wandbox.org/permlink/euLu4sbtPOFKnxBg

Copy link
Member

Choose a reason for hiding this comment

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

Ain't we already have those tests?

Good question! I was thinking about that actually. Was it in char1.cpp? Perhaps we can just augment those?

Copy link
Member Author

@cmazakas cmazakas May 5, 2019

Choose a reason for hiding this comment

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

Ah, Kojoley is correct.

… if you did not know it

At the time, I didn't. You are correct, the ASCII tests are repetitious in this case.

Should I simply move the Unicode tests here to the char1.cpp test file?

Copy link
Member

Choose a reason for hiding this comment

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

At the time, I didn't. You are correct, the ASCII tests are repetitious in this case.

Should I simply move the Unicode tests here to the char1.cpp test file?

Yes please :-)

Copy link
Member Author

Choose a reason for hiding this comment

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

Done!

@cmazakas cmazakas reopened this May 7, 2019
@cmazakas
Copy link
Member Author

cmazakas commented May 7, 2019

For some reason, the Travis CI link is showing the wrong build job. This is the correct one that passes for the latest changes:
https://travis-ci.org/boostorg/spirit/builds/529120733

@djowel
Copy link
Member

djowel commented May 7, 2019

For some reason, the Travis CI link is showing the wrong build job. This is the correct one that passes for the latest changes:
https://travis-ci.org/boostorg/spirit/builds/529120733

We should be good.

@djowel djowel merged commit 3d0dafe into boostorg:develop May 7, 2019
@djowel
Copy link
Member

djowel commented May 7, 2019

Merged. Many thanks for the splendid PR.

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.

None yet

3 participants