You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@mesos.apache.org by Michael Park <mp...@apache.org> on 2017/01/11 10:03:50 UTC
Review Request 55410: Used standard classic locale for `jsonify`.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55410/
-----------------------------------------------------------
Review request for mesos and Alexander Rojas.
Repository: mesos
Description
-------
The `<xlocale.h>` header doesn't exist in Windows and thus broke
the Windows build.
```
fatal error C1083: Cannot open include file: 'xlocale.h': No such file
```
Diffs
-----
3rdparty/stout/include/stout/jsonify.hpp 3c48046e087de2a66139a31449327fd94c149371
Diff: https://reviews.apache.org/r/55410/diff/
Testing
-------
Thanks,
Michael Park
Re: Review Request 55410: Used standard classic locale for `jsonify`.
Posted by Mesos ReviewBot <re...@mesos.apache.org>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55410/#review161214
-----------------------------------------------------------
Patch looks great!
Reviews applied: [55410]
Passed command: export OS='ubuntu:14.04' BUILDTOOL='autotools' COMPILER='gcc' CONFIGURATION='--verbose' ENVIRONMENT='GLOG_v=1 MESOS_VERBOSE=1'; ./support/docker_build.sh
- Mesos ReviewBot
On Jan. 11, 2017, 10:03 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55410/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2017, 10:03 a.m.)
>
>
> Review request for mesos and Alexander Rojas.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `<xlocale.h>` header doesn't exist in Windows and thus broke
> the Windows build.
>
> ```
> fatal error C1083: Cannot open include file: 'xlocale.h': No such file
> ```
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/jsonify.hpp 3c48046e087de2a66139a31449327fd94c149371
>
> Diff: https://reviews.apache.org/r/55410/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 55410: Used standard classic locale for `jsonify`.
Posted by Michael Park <mp...@apache.org>.
> On Jan. 11, 2017, 3:05 a.m., Alexander Rojas wrote:
> > This patch doesn't really fix the issue, the following program fails to produce appropriate JSON:
> >
> > ```c++
> > #include <stout/json.hpp>
> > #include <stout/jsonify.hpp>
> >
> > #include <clocale>
> > #include <iostream>
> > #include <locale>
> >
> > int main(int argc, char **argv) {
> > // Set locale to German.
> > std::setlocale(LC_ALL, "de_DE.UTF-8");
> > std::cout.imbue(std::locale("de_DE.UTF-8"));
> >
> > std::vector<double> doubles = {1234567890.12345, 123456789.012345};
> > std::cout << jsonify(doubles) << '\n';
> >
> > std::string str = jsonify(doubles);
> > std::cout << str << '\n';
> > return 0;
> > }
> > ```
> >
> > The reason is that the patch changes the locale of the `stream` but printing is done with `snprintf()`. At the same time, changing the locale of the stream is not thread safe (although using the stream concurrently should be heavily discouraged).
Synced with Alexander on Slack, we're going to implement `uselocale` on Windows, similar to how libc++ does here: https://github.com/llvm-mirror/libcxx/blob/1b34b986bcc7e0991513213c295f3c9c82072a34/src/support/win32/locale_win32.cpp#L26-L39
- Michael
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55410/#review161209
-----------------------------------------------------------
On Jan. 11, 2017, 2:03 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55410/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2017, 2:03 a.m.)
>
>
> Review request for mesos and Alexander Rojas.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `<xlocale.h>` header doesn't exist in Windows and thus broke
> the Windows build.
>
> ```
> fatal error C1083: Cannot open include file: 'xlocale.h': No such file
> ```
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/jsonify.hpp 3c48046e087de2a66139a31449327fd94c149371
>
> Diff: https://reviews.apache.org/r/55410/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>
Re: Review Request 55410: Used standard classic locale for `jsonify`.
Posted by Alexander Rojas <al...@mesosphere.io>.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://reviews.apache.org/r/55410/#review161209
-----------------------------------------------------------
This patch doesn't really fix the issue, the following program fails to produce appropriate JSON:
```c++
#include <stout/json.hpp>
#include <stout/jsonify.hpp>
#include <clocale>
#include <iostream>
#include <locale>
int main(int argc, char **argv) {
// Set locale to German.
std::setlocale(LC_ALL, "de_DE.UTF-8");
std::cout.imbue(std::locale("de_DE.UTF-8"));
std::vector<double> doubles = {1234567890.12345, 123456789.012345};
std::cout << jsonify(doubles) << '\n';
std::string str = jsonify(doubles);
std::cout << str << '\n';
return 0;
}
```
The reason is that the patch changes the locale of the `stream` but printing is done with `snprintf()`. At the same time, changing the locale of the stream is not thread safe (although using the stream concurrently should be heavily discouraged).
- Alexander Rojas
On Jan. 11, 2017, 11:03 a.m., Michael Park wrote:
>
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://reviews.apache.org/r/55410/
> -----------------------------------------------------------
>
> (Updated Jan. 11, 2017, 11:03 a.m.)
>
>
> Review request for mesos and Alexander Rojas.
>
>
> Repository: mesos
>
>
> Description
> -------
>
> The `<xlocale.h>` header doesn't exist in Windows and thus broke
> the Windows build.
>
> ```
> fatal error C1083: Cannot open include file: 'xlocale.h': No such file
> ```
>
>
> Diffs
> -----
>
> 3rdparty/stout/include/stout/jsonify.hpp 3c48046e087de2a66139a31449327fd94c149371
>
> Diff: https://reviews.apache.org/r/55410/diff/
>
>
> Testing
> -------
>
>
> Thanks,
>
> Michael Park
>
>