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

UriParser improvements #743

Merged
merged 3 commits into from
Jul 16, 2019
Merged

UriParser improvements #743

merged 3 commits into from
Jul 16, 2019

Conversation

AdeMiller
Copy link
Contributor

  • Make property accessors const.
  • Proto is converted to lower case. The current code assumes a lowercase
    proto and will fail if the input is not lowercase.
  • Type now supports all existing types.
  • Type RTP added.
  • Test code no longer crashes when called with no args.
  • Fixed layout removing extraneous empty lines.

* Make property accessors const.
* Proto is converted to lower case. The current code assumes a lowercase
proto and will fail if the input is not lowercase.
* Type now supports all existing types.
* Type RTP added.
* Test code no longer crashes when called with no args.
* Fixed layout removing extraneous empty lines.
@maxsharabayko maxsharabayko added this to the v.1.3.3 milestone Jul 1, 2019
@maxsharabayko maxsharabayko added the [apps] Area: Test applications related improvements label Jul 1, 2019
apps/uriparser.cpp Outdated Show resolved Hide resolved
@ethouris
Copy link
Collaborator

ethouris commented Jul 1, 2019

@AdeMiller : I was kinda surprised that this call with std::transform works; I had problem once that was caused by having two overloaded versions of ::tolower, with one extra definition for the sake of locales. Nice if you check if this still gets compiled if you add #include <locale> in the beginning of the file. Might be that it needs an explicit specification of kinda wrapper struct ToLower { char operator()(char c) { return ::tolower(c); } };. Unfortunately in C++ some headers might be included on a whim by particular compiler, changing the rules this way.

Otherwise, beside these braces, I have no more comments.

@maxsharabayko maxsharabayko self-requested a review July 1, 2019 09:57
@AdeMiller
Copy link
Contributor Author

AdeMiller commented Jul 1, 2019

@AdeMiller : I was kinda surprised that this call with std::transform works; I had problem once that was caused by having two overloaded versions of ::tolower, with one extra definition for the sake of locales. Nice if you check if this still gets compiled if you add #include <locale> in the beginning of the file. Might be that it needs an explicit specification of kinda wrapper struct ToLower { char operator()(char c) { return ::tolower(c); } };. Unfortunately in C++ some headers might be included on a whim by particular compiler, changing the rules this way.

Otherwise, beside these braces, I have no more comments.

So I did some investigation around this ::tolower forces the outermost namespace so works fine with an include like locale. However if the code references std::tolower then including locale will cause compilation errors in exactly the way you describe. It seems like the most robust fix for this would be to explicitly reference the STL function from within a lambda

        transform(m_proto.begin(), m_proto.end(), m_proto.begin(), [](char c){ return std::toupper(c); });

I've changed the code accordingly.

apps/uriparser.cpp Outdated Show resolved Hide resolved
apps/uriparser.cpp Outdated Show resolved Hide resolved
Copy link
Collaborator

@ethouris ethouris left a comment

Choose a reason for hiding this comment

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

Please update according to the comments.

@ethouris ethouris modified the milestones: v.1.3.3, v.1.3.4 Jul 5, 2019
@rndi rndi merged commit 92af4e1 into Haivision:master Jul 16, 2019
@rndi rndi modified the milestones: v.1.3.4, v.1.3.3 Jul 16, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
[apps] Area: Test applications related improvements
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants