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

Make the Parser independent from the global C-locale #5028

Merged
merged 3 commits into from
Nov 16, 2018

Conversation

vglavnyy
Copy link
Contributor

This PR adds detection of locale-independent functions strdtod_l, strdtof_l, strdtoll_l, strdtoull_l instead of locale-wide strdtod, strdtoll, strdtoull .
After this PR the IDL parser will be fully compatible with the ASCII alphabet of the Flatbuffers grammar.

There are some direct calls of strdtod in the Flatbuffers code, these calls are not related to the parser directly (flexbuffer, reflection).

The parse has direct calls of local-wide atoi function.
A locale-safety of these calls requires investigation (however, all tests passed).

Minor changes:

  1. Speed-up travis-ci builds using parallel compilation
  2. Minor bug fixes in scalar_fuzzer.cc code

@vglavnyy vglavnyy force-pushed the local_independent_parser branch from efddddf to c5cbdfe Compare November 11, 2018 12:42
@vglavnyy vglavnyy force-pushed the local_independent_parser branch from 3e9765d to 3fc7cec Compare November 12, 2018 01:25
@aardappel
Copy link
Collaborator

Generally lookls good to me, thanks! Ready to go in?

@vglavnyy
Copy link
Contributor Author

I think better replace the hard-coded FLATBUFFERS_TEST_LOCALE by an environment variable:

Enable test of locale independent code.
-DFLATBUFFERS_TEST_LOCALE="" - test with default C-locale
-DFLATBUFFERS_TEST_LOCALE="ru_RU.CP1251" - test with ru_RU.CP1251

to

  1. std::getenv("FBS_TEST_LOCALE"); - in a code (since C++11)
  2. FBS_TEST_LOCALE="ru_RU.CP1251" ./flattests - in a console
  3. FBS_TEST_OPTIONS=LOCALE="ru_RU.CP1251" ./flattests - like ASAN/UBSAN

@aardappel
Copy link
Collaborator

Hmm, if you're sure this getenv works on all platforms, it is worth trying. Remember we support VS2010 (C++0x) and Android with STLPort. Rather than #ifdeffing those out, your current code may be more universal?

@vglavnyy
Copy link
Contributor Author

vglavnyy commented Nov 16, 2018

Have done.
std::getenv passed all builds without #ifdef.

Additionally:

  • some code have been formatted (test_assert.[cpp && h], util.h);
  • the idl_parser.cpp file in the master branch has format UTF-8 with BOM while others files have UTF-8 without BOM. Current commit saved the file as simple UTF-8 without BOM;

Question:
Should the functions in util.h be inline: SaveFile, StripExtension, GetExtension, StripPath, StripFileName, ConCatPathFileName and etc?
Some #include directives may be removed from util.h if move these functions to util.cpp.

@vglavnyy
Copy link
Contributor Author

VS2013-Release-x86 Rust failed with message memory allocation of 2147483648 bytes failederror: test failed, to rerun pass '--test integration_test'.

I have no idea why this error occurred.

@aardappel
Copy link
Collaborator

Thanks for the other fixes.
The Rust error is unrelated.
Yes, it be great to move some functionality in util.h to util.cpp to move #includes, in particular windows.h

@aardappel
Copy link
Collaborator

Merging this, util.h refactor can go in other PR.

@aardappel aardappel merged commit 5f32f94 into google:master Nov 16, 2018
@vglavnyy vglavnyy deleted the local_independent_parser branch November 17, 2018 00:54
zchee pushed a commit to zchee/flatbuffers that referenced this pull request Feb 14, 2019
* Make the Parser independent from the global C-locale

* Set a specific test locale using the environment variable FLATBUFFERS_TEST_LOCALE

* Remove redundant static qualifiers
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants