8000 fixes issue 891 (#893) · JavaScriptExpert/simdjson@40d57da · GitHub
[go: up one dir, main page]
Skip to content

Commit 40d57da

Browse files
lemirelemire
authored
fixes issue 891 (simdjson#893)
1 parent 561813e commit 40d57da

File tree

8 files changed

+63
-23
lines changed
Collapse file tree

8 files changed

+63
-23
lines changed

‎CONTRIBUTING.md

Lines changed: 13 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -5,7 +5,7 @@ The simdjson library is an open project written in C++. Contributions are invite
55
agree to the project's license.
66

77
We have an extensive list of issues, and contributions toward any of these issues is invited.
8-
Contributions can take the form of code samples, better documentation or design ideas.
8+
Contributions can take the form of code samples, better documentation or design ideas.
99

1010
In particular, the following contributions are invited:
1111

@@ -32,6 +32,17 @@ We discourage the following types of contributions:
3232

3333
In short, most code changes should either bring new features or better performance. We want to avoid unmotivated code changes.
3434

35+
36+
Specific rules
37+
----------
38+
39+
We have few hard rules, but we have some:
40+
41+
- Printing to standard output or standard error (`stderr`, `stdout`, `std::cerr`, `std::cout`) in the core library is forbidden. This follows from the [Writing R Extensions](https://cran.r-project.org/doc/manuals/R-exts.html) manual which states that "Compiled code should not write to stdout or stderr".
42+
- Calls to `abort()` are forbidden in the core library. This follows from the [Writing R Extensions](https://cran.r-project.org/doc/manuals/R-exts.html) manual which states that "Under no circumstances should your compiled code ever call abort or exit".
43+
44+
Tools, tests and benchmarks are not held to these same strict rules.
45+
3546
General Guidelines
3647
----------
3748

@@ -43,7 +54,6 @@ Contributors are encouraged to :
4354
- Tools may report "problems" with the code, but we never delegate programming to tools: if there is a problem with the code, we need to understand it. Thus we will not "fix" code merely to please a static analyzer if we do not understand.
4455
- Provide tests for any new feature. We will not merge a new feature without tests.
4556

46-
4757
Pull Requests
4858
--------------
4959

@@ -68,7 +78,7 @@ intimidation. Everyone is welcome to contribute. If you have concerns, you can r
6878

6979
We welcome contributions from women and less represented groups. If you need help, please reach out.
7080

71-
Consider the following points when engaging with the project:
81+
Consider the following points when engaging with the project:
7282

7383
- We discourage arguments from authority: ideas are discusssed on their own merits and not based on who stated it.
7484
- Be mindful that what you may view as an aggression is maybe merely a difference of opinion or a misunderstanding.

‎include/simdjson/inline/element.h

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -293,7 +293,7 @@ inline std::ostream& operator<<(std::ostream& out, element_type type) {
293293
case element_type::NULL_VALUE:
294294
return out << "null";
295295
default:
296-
abort();
296+
return out << "unexpected content!!!"; // abort() usage is forbidden in the library
297297
}
298298
}
299299

@@ -405,7 +405,7 @@ inline std::ostream& minify<dom::element>::print(std::ostream& out) {
405405
case tape_type::END_ARRAY:
406406
case tape_type::END_OBJECT:
407407
case tape_type::ROOT:
408-
abort();
408+
out << "unexpected content!!!"; // abort() usage is forbidden in the library
409409
}
410410
iter.json_index++;
411411
after_value = true;
@@ -438,4 +438,4 @@ inline std::ostream& operator<<(std::ostream& out, const simdjson_result<dom::el
438438

439439
} // namespace simdjson
440440

441-
#endif // SIMDJSON_INLINE_ELEMENT_H
441+
#endif // SIMDJSON_INLINE_ELEMENT_H

‎include/simdjson/inline/parsedjson_iterator.h

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -220,7 +220,7 @@ dom::parser::Iterator::Iterator(const dom::parser &pj) noexcept(false)
220220
#if SIMDJSON_EXCEPTIONS
221221
if (!pj.valid) { throw simdjson_error(pj.error); }
222222
#else
223-
if (!pj.valid) { abort(); }
223+
if (!pj.valid) { return; } // abort() usage is forbidden in the library
224224
#endif
225225

226226
max_depth = pj.max_depth();

‎singleheader/amalgamate_demo.cpp

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
/* auto-generated on Tue May 19 14:39:19 PDT 2020. Do not edit! */
1+
/* auto-generated on Wed May 20 10:23:07 EDT 2020. Do not edit! */
22

33
#include <iostream>
44
#include "simdjson.h"

‎singleheader/simdjson.cpp

Lines changed: 7 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
/* auto-generated on Tue May 19 14:39:19 PDT 2020. Do not edit! */
1+
/* auto-generated on Wed May 20 10:23:07 EDT 2020. Do not edit! */
22
/* begin file src/simdjson.cpp */
33
#include "simdjson.h"
44

@@ -569,7 +569,7 @@ const implementation *available_implementation_list::detect_best_supported() con
569569
uint32_t required_instruction_sets = impl->required_instruction_sets();
570570
if ((supported_instruction_sets & required_instruction_sets) == required_instruction_sets) { return impl; }
571571
}
572-
return &unsupported_singleton;
572+
return &unsupported_singleton; // this should never happen?
573573
}
574574

575575
const implementation *detect_best_supported_implementation_on_first_use::set_best() const noexcept {
@@ -580,11 +580,12 @@ const implementation *detect_best_supported_implementation_on_first_use::set_bes
580580

581581
if (force_implementation_name) {
582582
auto force_implementation = available_implementations[force_implementation_name];
583-
if (!force_implementation) {
584-
fprintf(stderr, "SIMDJSON_FORCE_IMPLEMENTATION environment variable set to '%s', which is not a supported implementation name!\n", force_implementation_name);
585-
abort();
583+
if (force_implementation) {
584+
return active_implementation = force_implementation;
585+
} else {
586+
// Note: abort() and stderr usage within the library is forbidden.
587+
return active_implementation = &unsupported_singleton;
586588
}
587-
return active_implementation = force_implementation;
588589
}
589590
return active_implementation = available_implementations.detect_best_supported();
590591
}

‎singleheader/simdjson.h

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
/* auto-generated on Tue May 19 14:39:19 PDT 2020. Do not edit! */
1+
/* auto-generated on Wed May 20 10:23:07 EDT 2020. Do not edit! */
22
/* begin file include/simdjson.h */
33
#ifndef SIMDJSON_H
44
#define SIMDJSON_H
@@ -5552,7 +5552,7 @@ inline std::ostream& operator<<(std::ostream& out, element_type type) {
55525552
case element_type::NULL_VALUE:
55535553
return out << "null";
55545554
default:
5555-
abort();
5555+
return out << "unexpected content!!!"; // abort() usage is forbidden in the library
55565556
}
55575557
}
55585558

@@ -5664,7 +5664,7 @@ inline std::ostream& minify<dom::element>::print(std::ostream& out) {
56645664
case tape_type::END_ARRAY:
56655665
case tape_type::END_OBJECT:
56665666
case tape_type::ROOT:
5667-
abort();
5667+
out << "unexpected content!!!"; // abort() usage is forbidden in the library
56685668
}
56695669
iter.json_index++;
56705670
after_value = true;
@@ -6461,7 +6461,7 @@ dom::parser::Iterator::Iterator(const dom::parser &pj) noexcept(false)
64616461
#if SIMDJSON_EXCEPTIONS
64626462
if (!pj.valid) { throw simdjson_error(pj.error); }
64636463
#else
6464-
if (!pj.valid) { abort(); }
6464+
if (!pj.valid) { return; } // abort() usage is forbidden in the library
64656465
#endif
64666466

64676467
max_depth = pj.max_depth();

‎src/CMakeLists.txt

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,34 @@ endif()
4545
target_link_libraries(simdjson PUBLIC simdjson-headers simdjson-flags) # Only expose the headers, not sources
4646
target_link_libraries(simdjson PRIVATE simdjson-source simdjson-internal-flags)
4747

48+
##
49+
## In systems like R, libraries must not use stderr or abort to be acceptable.
50+
## Thus we make it a hard rule that one is not allowed to call abort or stderr.
51+
## The sanitized builds are allowed to abort.
52+
##
53+
if(NOT SIMDJSON_SANITIZE)
54+
find_program(GREP grep)
55+
find_program(NM nm)
56+
if((NOT GREP) OR (NOT NM))
57+
message("grep and nm are unavailable on this system.")
58+
else()
59+
add_test(
60+
NAME "avoid_abort"
61+
COMMAND sh -c "${NM} $<TARGET_FILE_NAME:simdjson> | ${GREP} abort || exit 0 && exit 1"
62+
WORKING_DIRECTORY ${PROJECT_BINARY_DIR}
63+
)
64+
add_test(
65+
NAME "avoid_stdout"
66+
COMMAND sh -c "${NM} $<TARGET_FILE_NAME:simdjson> | ${GREP} stdout || exit 0 && exit 1"
67+
WORKING_DIRECTORY ${PROJECT_BINARY_DIR}
68+
)
69+
add_test(
70+
NAME "avoid_stderr"
71+
COMMAND sh -c "${NM} $<TARGET_FILE_NAME:simdjson> | ${GREP} stderr || exit 0 && exit 1"
72+
WORKING_DIRECTORY ${PROJECT_BINARY_DIR}
73+
)
74+
endif()
75+
endif()
4876

4977
if(NOT MSVC)
5078
## We output the library at the root of the current directory where cmake is invoked

‎src/implementation.cpp

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -118,7 +118,7 @@ const implementation *available_implementation_list::detect_best_supported() con
118118
uint32_t required_instruction_sets = impl->required_instruction_sets();
119119
if ((supported_instruction_sets & required_instruction_sets) == required_instruction_sets) { return impl; }
120120
}
121-
return &unsupported_singleton;
121+
return &unsupported_singleton; // this should never happen?
122122
}
123123

124124
const implementation *detect_best_supported_implementation_on_first_use::set_best() const noexcept {
@@ -129,11 +129,12 @@ const implementation *detect_best_supported_implementation_on_first_use::set_bes
129129

130130
if (force_implementation_name) {
131131
auto force_implementation = available_implementations[force_implementation_name];
132-
if (!force_implementation) {
133-
fprintf(stderr, "SIMDJSON_FORCE_IMPLEMENTATION environment variable set to '%s', which is not a supported implementation name!\n", force_implementation_name);
134-
abort();
132+
if (force_implementation) {
133+
return active_implementation = force_implementation;
134+
} else {
135+
// Note: abort() and stderr usage within the library is forbidden.
136+
return active_implementation = &unsupported_singleton;
135137
}
136-
return active_implementation = force_implementation;
137138
}
138139
return active_implementation = available_implementations.detect_best_supported();
139140
}

0 commit comments

Comments
 (0)
0