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

Fix syntax error in single_image_random_dot_stereograms caused by locale #22044

Merged
merged 4 commits into from
Sep 10, 2018

Conversation

yongtang
Copy link
Member

@yongtang yongtang commented Sep 4, 2018

This fix tries to address the issue raised in #21164 where the single_image_random_dot_stereograms in different locale (like de_DE) caused syntax error in python like:

File "<string>", line 28
    def single_image_random_dot_stereograms(depth_values, hidden_surface_removal=True, convergence_dots_size=8, dots_per_inch=72, eye_separation=2,5, mu=0,333299994, normalize=True, normalize_max=-100, normalize_min=100, border_level=0, number_colors=256, output_image_shape=[1024, 768, 1], output_data_window=[1022, 757], name=None):
                                                                                                                                      ^
SyntaxError: invalid syntax

The issue was that the float to string conversion in python_op_gen_internal.cc triggered snprintf (in FloatToBuffer) which is local dependent and generates something like eye_separatiion=2,5 in DE locale.

This fix replaced the float to string conversion with locale-independent

      std::ostringstream s;
      s.imbue(std::locale::classic());

This fix fixes #21164.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

This fix tries to address the issue raised in 21164 where
the single_image_random_dot_stereograms in different locale
(like de_DE) caused syntax error in python like:
```
File "<string>", line 28
    def single_image_random_dot_stereograms(depth_values, hidden_surface_removal=True, convergence_dots_size=8, dots_per_inch=72, eye_separation=2,5, mu=0,333299994, normalize=True, normalize_max=-100, normalize_min=100, border_level=0, number_colors=256, output_image_shape=[1024, 768, 1], output_data_window=[1022, 757], name=None):
                                                                                                                                      ^
SyntaxError: invalid syntax
```

The issue was that the float to string conversion in
python_op_gen_internal.cc triggered snprintf (in `FloatToBuffer`) which is local
dependent and generates something like `eye_separatiion=2,5` in DE locale.

This fix replaced the float to string conversion with locale-independent
```
      std::ostringstream s;
      s.imbue(std::locale::classic());
```

This fix fixes 21164.

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
martinwicke
martinwicke previously approved these changes Sep 4, 2018
@martinwicke
Copy link
Member

Nice.

@martinwicke martinwicke added the kokoro:force-run Tests on submitted change label Sep 4, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 4, 2018
@martinwicke
Copy link
Member

Wait, your last commit already ran clang-format? Is that stale?

@martinwicke
Copy link
Member

Is this a difference between clang-format versions? I'm afraid of those...

to conform to `Experimental clang-format Check`

Signed-off-by: Yong Tang <yong.tang.github@outlook.com>
@yongtang
Copy link
Member Author

yongtang commented Sep 5, 2018

Thanks @martinwicke for the review. The PR has been updated to address the issue in Experimental clang-format Check failure.

The clang-format might be different over versions. I am wondering if there is some information about platform, clang-format version, and the command line in Experimental clang-format Check? That probably helps so that developer could run the Experimental clang-format Check before submit the PR.

@yongtang
Copy link
Member Author

yongtang commented Sep 5, 2018

Tried several different versions of clang-format, it looks like clang-format installed through Ubuntu 16.04 almost matches the behavior of Experimental clang-format Check. except that the following line

#include "third_party/eigen3/unsupported/Eigen/CXX11/Tensor"

will be moved and sorted in clang-format -i --style=Google, while Experimental clang-format Check will keep the above line at the beginning of the included headers.

I remember placing unsupported/Eigen/CXX11/Tensor was intentional and had a discussion before. Did a search but couldn't find the discussion and related PR though.

@martinwicke
Copy link
Member

@yongtang I'll pull this in and hope for the best.

@yifeif This is the dreaded lint problem. Which version of clang-format are we using exactly?

@martinwicke martinwicke added the kokoro:force-run Tests on submitted change label Sep 5, 2018
@kokoro-team kokoro-team removed the kokoro:force-run Tests on submitted change label Sep 5, 2018
@yifeif
Copy link
Contributor

yifeif commented Sep 5, 2018

We are using 3.9 with -style=google in Experimental clang-format Check. We can add this information to the failure log in addition to the diff. @martinwicke pulling should be okay.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cla: yes ready to pull PR ready for merge process
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Default floating point values in single_image_random_dot_stereograms_ops.cc cause syntax error
6 participants