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.

Sign up for GitHub

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

added URI resolution according to RFC3986 #897

Merged
merged 6 commits into from
Oct 10, 2018
Merged

added URI resolution according to RFC3986 #897

Changes from 1 commit
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
557b025
added URI resolution according to RFC3986, Section 5 (https://tools.i…
Oct 9, 2018
a34749c
fixed std::wstring -> utility::string_t
Oct 9, 2018
a218343
fixed literals
Oct 9, 2018
f78bc69
implemented suggestions by @BillyONeal;
Oct 10, 2018
f0d67cc
Make dotSegment and dotDotSegment global constants to pay for their c…
BillyONeal Oct 10, 2018
7963710
Use push_back instead of initializer_list, minor whitespace fixups.
BillyONeal Oct 10, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Prev Previous commit
Next Next commit
fixed literals
  • Loading branch information
Pavel Novikov committed Oct 9, 2018
commit a218343ac45541ba8b9dd959bf1ed46b9f84dda0
16 changes: 8 additions & 8 deletions Release/src/uri/uri.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -426,10 +426,10 @@ namespace
// 5.2.3. Merge Paths https://tools.ietf.org/html/rfc3986#section-5.2.3
utility::string_t mergePaths(const utility::string_t &base, const utility::string_t &relative)
BillyONeal marked this conversation as resolved.
Show resolved Hide resolved
{
const auto lastSlash = base.rfind(L'/');
const auto lastSlash = base.rfind(_XPLATSTR('/'));
if (lastSlash == utility::string_t::npos)
{
return base + L'/' + relative;
return base + _XPLATSTR('/') + relative;
}
else if (lastSlash == base.size() - 1)
{
Expand All @@ -442,7 +442,7 @@ namespace
// 5.2.4. Remove Dot Segments https://tools.ietf.org/html/rfc3986#section-5.2.4
void removeDotSegments(web::uri_builder &builder)
{
if (builder.path().find(L'.') == utility::string_t::npos)
if (builder.path().find(_XPLATSTR('.')) == utility::string_t::npos)
return;

const auto segments = web::uri::split_path(builder.path());
Expand All @@ -464,9 +464,9 @@ namespace
utility::stringstream_t path;
Copy link
Member

Choose a reason for hiding this comment

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

It seems like this would be better with string_t's operator+ or +=?

Copy link
Contributor Author

@toughengineer toughengineer Oct 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Ultimately either the string or the stringstream will manage the buffer growth so it is roughly the same.
I consider stringstream's memory management to be somewhat better because it is supposed to grow by its very purpose, and the interface is more convenient (e.g. you can operator<< numbers, though not in this case).
It's all arguable, however.

Copy link
Member

Choose a reason for hiding this comment

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

I can tell you as someone who has maintained MSVC++'s stringstream that it is far less efficient than std::string. Like "virtual function calls per character" less efficient. If you need number formatting or something like that with it then of course stringstream is the only option, but this is just string concat.

Copy link
Contributor Author

@toughengineer toughengineer Oct 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

You are right, I forgot about that design feature of streams. Will fix it.

path << result.front().get();
for (size_t i = 1; i != result.size(); ++i)
path << L'/' << result[i].get();
if (segments.back() == L".." || segments.back() == L"." || builder.path().back() == L'/')
path << L'/';
path << _XPLATSTR('/') << result[i].get();
if (segments.back() == _XPLATSTR("..") || segments.back() == _XPLATSTR(".") || builder.path().back() == _XPLATSTR('/'))
path << _XPLATSTR('/');

builder.set_path(path.str());
}
Expand Down Expand Up @@ -840,7 +840,7 @@ utility::string_t uri::resolve_uri(const utility::string_t &relativeUri) const
if (relativeUri[0] == _XPLATSTR('/')) // starts with '/'
{
if (relativeUri.size() >= 2 && relativeUri[1] == _XPLATSTR('/')) // starts with '//'
return scheme() + L':' + relativeUri;
return scheme() + _XPLATSTR(':') + relativeUri;

// otherwise relative to root
auto builder = web::uri_builder(this->authority());
Expand All @@ -858,7 +858,7 @@ utility::string_t uri::resolve_uri(const utility::string_t &relativeUri) const

// relative url
auto builder = web::uri_builder(*this);
if (url.path() == L"/" || url.path().empty()) // web::uri considers empty path as '/'
if (url.path() == _XPLATSTR("/") || url.path().empty()) // web::uri considers empty path as '/'
{
if (!url.query().empty())
builder.set_query(url.query());
Expand Down