You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Andrew Schwartzmeyer <an...@schwartzmeyer.com> on 2017/03/31 23:45:33 UTC
Review Request 58125: Stout: Added stringify for std::wstring.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58125/
-----------------------------------------------------------
Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, Li Li, and Michael Park.
Repository: mesos
Description
-------
Stout: Added stringify for std::wstring.
Diffs
-----
3rdparty/stout/include/stout/stringify.hpp e9588d8d940046791794100c53469288656a14f0
Diff: https://reviews.apache.org/r/58125/diff/1/
Testing
-------
Testing done later in chain.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 58125: Stout: Added stringify for std::wstring.
Posted by Joseph Wu <jo...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58125/#review171069
-----------------------------------------------------------
Ship it!
3rdparty/stout/include/stout/stringify.hpp
Lines 47-48 (patched)
<https://reviews.apache.org/r/58125/#comment243913>
As Benjamin suggested, I'll make this into an override (rather than a template specialization) before committing so that we can take a `const &` instead of a `const`.
- Joseph Wu
On March 31, 2017, 9:32 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58125/
> -----------------------------------------------------------
>
> (Updated March 31, 2017, 9:32 p.m.)
>
>
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, Li Li, and Michael Park.
>
>
> Bugs: MESOS-5418
> https://issues.apache.org/jira/browse/MESOS-5418
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Added stringify for std::wstring.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/stringify.hpp e9588d8d940046791794100c53469288656a14f0
>
>
> Diff: https://reviews.apache.org/r/58125/diff/2/
>
>
> Testing
> -------
>
> Testing done later in chain.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 58125: Stout: Added stringify for std::wstring.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58125/
-----------------------------------------------------------
(Updated April 1, 2017, 4:32 a.m.)
Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, Li Li, and Michael Park.
Changes
-------
Guard addition for non-Windows platforms until we figure out `codecvt` on downstream distros.
Repository: mesos
Description
-------
Stout: Added stringify for std::wstring.
Diffs (updated)
-----
3rdparty/stout/include/stout/stringify.hpp e9588d8d940046791794100c53469288656a14f0
Diff: https://reviews.apache.org/r/58125/diff/2/
Changes: https://reviews.apache.org/r/58125/diff/1-2/
Testing
-------
Testing done later in chain.
Thanks,
Andrew Schwartzmeyer
Re: Review Request 58125: Stout: Added stringify for std::wstring.
Posted by Benjamin Bannier <be...@mesosphere.io>.
> On April 1, 2017, 1:46 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/stringify.hpp
> > Lines 42 (patched)
> > <https://reviews.apache.org/r/58125/diff/1/?file=1682876#file1682876line42>
> >
> > I should probably have taken a `const std::wstring&` here. Also, is the `<std::wstring>` 100% necessary?
>
> Andrew Schwartzmeyer wrote:
> * I cannot take a const reference here as the usages of `stringify` receive `std::wstring` (for some reason the specialization isn't being picked up for a `const &`, I assume Micahel Park can tell me why, though it's happy to be `const`ed).
> * The `<std::wstring>` is not necessary.
The existing `stringify` functions are a mix of template specializations of `template <typename T> string stringify(T)` (for e.g., `bool`), and overloads (e.g., the ones for containers). If you'd declare an overload you should be fine,
string stringify(const wstring&) { ... }
I believe we should fix the base template to take by const ref, see MESOS-6560, but maybe mpark has an idea if that would have drawbacks.
- Benjamin
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58125/#review170799
-----------------------------------------------------------
On April 1, 2017, 6:32 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58125/
> -----------------------------------------------------------
>
> (Updated April 1, 2017, 6:32 a.m.)
>
>
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, Li Li, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Added stringify for std::wstring.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/stringify.hpp e9588d8d940046791794100c53469288656a14f0
>
>
> Diff: https://reviews.apache.org/r/58125/diff/2/
>
>
> Testing
> -------
>
> Testing done later in chain.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 58125: Stout: Added stringify for std::wstring.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On March 31, 2017, 11:46 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/stringify.hpp
> > Lines 42 (patched)
> > <https://reviews.apache.org/r/58125/diff/1/?file=1682876#file1682876line42>
> >
> > I should probably have taken a `const std::wstring&` here. Also, is the `<std::wstring>` 100% necessary?
* I cannot take a const reference here as the usages of `stringify` receive `std::wstring` (for some reason the specialization isn't being picked up for a `const &`, I assume Micahel Park can tell me why, though it's happy to be `const`ed).
* The `<std::wstring>` is not necessary.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58125/#review170799
-----------------------------------------------------------
On April 1, 2017, 4:32 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58125/
> -----------------------------------------------------------
>
> (Updated April 1, 2017, 4:32 a.m.)
>
>
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, Li Li, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Added stringify for std::wstring.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/stringify.hpp e9588d8d940046791794100c53469288656a14f0
>
>
> Diff: https://reviews.apache.org/r/58125/diff/2/
>
>
> Testing
> -------
>
> Testing done later in chain.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 58125: Stout: Added stringify for std::wstring.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58125/#review170799
-----------------------------------------------------------
3rdparty/stout/include/stout/stringify.hpp
Lines 42 (patched)
<https://reviews.apache.org/r/58125/#comment243674>
I should probably have taken a `const std::wstring&` here. Also, is the `<std::wstring>` 100% necessary?
- Andrew Schwartzmeyer
On March 31, 2017, 11:45 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58125/
> -----------------------------------------------------------
>
> (Updated March 31, 2017, 11:45 p.m.)
>
>
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, Li Li, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Added stringify for std::wstring.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/stringify.hpp e9588d8d940046791794100c53469288656a14f0
>
>
> Diff: https://reviews.apache.org/r/58125/diff/1/
>
>
> Testing
> -------
>
> Testing done later in chain.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 58125: Stout: Added stringify for std::wstring.
Posted by Andrew Schwartzmeyer <an...@schwartzmeyer.com>.
> On April 1, 2017, 12:01 a.m., Jeff Coffler wrote:
> > 3rdparty/stout/include/stout/stringify.hpp
> > Lines 42 (patched)
> > <https://reviews.apache.org/r/58125/diff/1/?file=1682876#file1682876line42>
> >
> > We've learned, from other projects, that converting from string to/from wstring to/from utf8string can be VERY expensive operations, particularly when unicode is allowed. Certain unicode operations can be extraordinarily slow.
> >
> > Due to this, we learned that we're far better off just storing things in a CONSISTENT format (generally wstring, since that's fast for length operations and such) and then, if we need to convert for final transmission, convert once at the very end.
> >
> > How heavily will this function be used?
> >
> > And, of course, it's in stout, but if stout was better desogmed, it would be nicer to have a separate implementation rather than inlining code in a bunch of places.
This is an additional function template specialization. No conversions are being explicitly performed here.
That said, there is no way that Mesos is going to convert to `wstring` across its entire codebase. However, for Windows code, the recommended best practices are to use the Unicode versions of Windows APIs (as the non-Unicode versions are not UTF at all, they're the current "Windows code page" aka ANSI which could be almost any encoding). Since Windows returns UTF-16, we need a reliable way to convert to UTF-8 on-demand for Mesos, which this additional specialization of `stringify` provides.
This needs to be inlined because it is a specialization (e.g. not a template) in a header. If it is not inlined, then it gets defined in multiple objects and linking fails.
- Andrew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58125/#review170802
-----------------------------------------------------------
On April 1, 2017, 4:32 a.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58125/
> -----------------------------------------------------------
>
> (Updated April 1, 2017, 4:32 a.m.)
>
>
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, Li Li, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Added stringify for std::wstring.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/stringify.hpp e9588d8d940046791794100c53469288656a14f0
>
>
> Diff: https://reviews.apache.org/r/58125/diff/2/
>
>
> Testing
> -------
>
> Testing done later in chain.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>
Re: Review Request 58125: Stout: Added stringify for std::wstring.
Posted by Jeff Coffler <je...@taltos.com>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/58125/#review170802
-----------------------------------------------------------
3rdparty/stout/include/stout/stringify.hpp
Lines 42 (patched)
<https://reviews.apache.org/r/58125/#comment243680>
We've learned, from other projects, that converting from string to/from wstring to/from utf8string can be VERY expensive operations, particularly when unicode is allowed. Certain unicode operations can be extraordinarily slow.
Due to this, we learned that we're far better off just storing things in a CONSISTENT format (generally wstring, since that's fast for length operations and such) and then, if we need to convert for final transmission, convert once at the very end.
How heavily will this function be used?
And, of course, it's in stout, but if stout was better desogmed, it would be nicer to have a separate implementation rather than inlining code in a bunch of places.
- Jeff Coffler
On March 31, 2017, 11:45 p.m., Andrew Schwartzmeyer wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/58125/
> -----------------------------------------------------------
>
> (Updated March 31, 2017, 11:45 p.m.)
>
>
> Review request for mesos, Jeff Coffler, John Kordich, Joseph Wu, Li Li, and Michael Park.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> Stout: Added stringify for std::wstring.
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/stringify.hpp e9588d8d940046791794100c53469288656a14f0
>
>
> Diff: https://reviews.apache.org/r/58125/diff/1/
>
>
> Testing
> -------
>
> Testing done later in chain.
>
>
> Thanks,
>
> Andrew Schwartzmeyer
>
>