Re: Review Request 58125: Stout: Added stringify for std::wstring.

[Joseph Wu]

---
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)


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.

[Andrew Schwartzmeyer]

> On April 1, 2017, 12:01 a.m., Jeff Coffler wrote:
> > 3rdparty/stout/include/stout/stringify.hpp
> > Lines 42 (patched)
> > 
> >
> > 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.

[Benjamin Bannier]

> On April 1, 2017, 1:46 a.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/stringify.hpp
> > Lines 42 (patched)
> > 
> >
> > I should probably have taken a `const std::wstring&` here. Also, is the 
> > `` 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 `` is not necessary.

The existing `stringify` functions are a mix of template specializations of 
`template  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.

[Andrew Schwartzmeyer]

> On March 31, 2017, 11:46 p.m., Andrew Schwartzmeyer wrote:
> > 3rdparty/stout/include/stout/stringify.hpp
> > Lines 42 (patched)
> > 
> >
> > I should probably have taken a `const std::wstring&` here. Also, is the 
> > `` 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 `` 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.

[Andrew Schwartzmeyer]

---
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.

[Jeff Coffler]

---
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)


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
> 
>

Re: Review Request 58125: Stout: Added stringify for std::wstring.

[Andrew Schwartzmeyer]

---
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)


I should probably have taken a `const std::wstring&` here. Also, is the 
`` 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
> 
>