Several simple patches #200
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ToruNiina
reviewed
Sep 23, 2022
tests/test_parse_file.cpp
Outdated
Comment on lines
946
to
951
| const result_type string_rref = toml::parse(std::move(example)); | ||
|
|
||
| BOOST_TEST_MESSAGE("strings"); | ||
|
|
||
| #ifdef TOML11_HAS_STD_FILESYSTEM | ||
| const std::filesystem::path fname_path(fname_string.begin(), fname_string.end()); | ||
| const auto filesystem_path = toml::parse(fname_path); | ||
| const std::filesystem::path fname_path(example.begin(), example.end()); |
There was a problem hiding this comment.
Currently some of the CI tasks are failing in this test case. I didn't look into it deeper, but at a glance, here you use a value example that is already moved at L946 (actually fname_string_mut is introduced to avoid this). I'm not sure whether this causes the error, but at least we need to fix this.
There was a problem hiding this comment.
Sorry for the somewhat late reply, I've been on a short vacation. I'll look into this problem as soon as the most urgent problems at work are resolved.
There was a problem hiding this comment.
I have to admit that I am very confused about the new build failure.
[ 53%] Built target test_parse_array
[ 54%] Building CXX object tests/CMakeFiles/test_parse_table.dir/test_parse_table.cpp.o
In file included from /home/runner/work/toml11/toml11/toml/value.hpp:8,
from /home/runner/work/toml11/toml11/toml/get.hpp:9,
from /home/runner/work/toml11/toml11/tests/test_parse_table.cpp:1:
/home/runner/work/toml11/toml11/toml/exception.hpp: In constructor ‘toml::file_io_error::file_io_error(int, const string&, const string&)’:
/home/runner/work/toml11/toml11/toml/exception.hpp:17:66: error: ‘strerror’ is not a member of ‘std’
17 | : std::runtime_error(msg + " \"" + fname + "\": " + std::strerror(errnum)),
| ^~~~~~~~
make[2]: *** [tests/CMakeFiles/test_parse_table.dir/build.make:76: tests/CMakeFiles/test_parse_table.dir/test_parse_table.cpp.o] Error 1
make[1]: *** [CMakeFiles/Makefile2:666: tests/CMakeFiles/test_parse_table.dir/all] Error 2
make: *** [Makefile:146: all] Error 2
Not only does it seem unrelated to the recent change, I also cannot find the tokens strerror or file_io_error anywhere in the code base.
Do you have any ideas what could have gone wrong here? I couldn't reproduce the failure locally either.
There was a problem hiding this comment.
That error is not your fault. Recently I merged another PR, so the updated patch is applied to the new code and caused the error.
I have just fixed it ~3 hours ago. Could you merge the current master branch to your several-simple-patches branch? Hopefully, it will fix the error.
There was a problem hiding this comment.
Thanks for the explanation, that really got me confused. I have rebased my branch onto the current master branch now.
This patch addresses a static analysis issue reported by Cppcheck 2.9 where several classes in the toml/datetime.hpp header explicitly default all their special member functions, including the default constructor, but don't provide initializers for their data members. This might or might not have caused any observable surprising behavior but I agree with Cppcheck on this one in that an explicitly defaulted default constructor should be expected to initialize all data members. So let's do that.
This patch addresses a static analysis issue reported by Cppcheck 2.9 where an assertion in the toml/region.hpp header would compare two container's (that are known to be of type std::vector<char>) begin() and end() iterators in order to verify that they are the same. This assertion either passes or invokes undefined behavior. Which isn't technically wrong because calling code must always ensure that preconditions are met and assertions therefore pass anyway but it does make the value that is added by having the assertion in the first place marginal. Fortunately, the assertion was easy to rewrite: Just compare the container's address itself. This is well-defined regardless of whether the assertion will pass or fail.
This patch addresses a static analysis issue reported by Cppcheck 2.9 where several member functions of the toml::discard_comment class defined in the toml/comments.hpp header were implemented to deliberately dereference the null pointer returned unconditionally by the always-empty container's data() member function. This behavior wasn't technically wrong because those functions all have as precondition that the container is non-empty so they must never be called on an instance of toml::discard_comment but we can still be more helpful without adversely affecting code generation. Instead of dereferencing the null pointer, this patch has these functions call an inline private helper function which is defined to invoke __builtin_unreachable() if available "and then" throw an exception with a helpful error message. Even at the -O1 level, GCC will optimize the code under the assumption that the function will never be called (i.e. no assembly is emitted), making failure to ensure this undefined behavior exactly as if the null pointer had been dereferenced. However, static analysis will now understand the programmer's intent and remain silent. Furthermore, when using the -O0 or -Og levels, GCC won't optimize under this assumption so the exception will be thrown and might be helpful for debugging. Compilers that don't have __builtin_unreachable() won't get any help in determining that the function must not be called and will have to figure this out by analyzing the calling code -- which really shouldn't exist in the first place anyway as the whole point is that these functions must not be called.
Most unit test files checked UNITTEST_FRAMEWORK_LIBRARY_EXIST and adapted themselves accordingly to either use the header-only version or link with the library. Alas, eight files didn't so the project couldn't really be tested with the header-only version of that Boost library. This patch adds the missing pre-processor logic to the files that were missing it. The style of using no indentation after the '#' was followed from the existing unit test files. Some other files in this repository do indent their pre-processor logic, though. Since replicating the conditional in every file is kind of verbose, tedious and, apparently, easily forgotten, I'm wondering whether isolating that logic into a tiny little auxiliary header and then unconditionally including that one in each unit test file would be a good idea, though.
The conditional inclusion of either the library or the header-only
version of the Boost.Test header wasn't tremendously useful in practice
because the tests/CMakeLists.txt file would unconditionally add compile
definitions to basically fore dynamic linking.
This patch adds feature detection to the tests/CMakeLists.txt file to
determine whether to use dynamic linking, static linking or the
header-only version (in that order of preference, for best performance).
The automatic detection could be overridden if needed by defining the
TOML11_WITH_BOOST_TEST_{HEADER,STATIC,DYNAMIC}
variables on the CMake command line.
While we are at it, instead of having a conditional
#define BOOST_TEST_NO_LIB
in each unit test file, handle this once in the CMakeLists.txt file.
Depending on the CMake and Boost version, the -Werror flags that might get added to CMAKE_CXX_FLAGS could adversely affect the feature detection logic, leading to the wrong conclusion that Boost.Test isn't usable altogether. Performing these checks first and only afterwards altering CMAKE_CXX_FLAGS avoids this issue.
Instead of unconditionally attempting to clone from a fixed location
(GitHub) during the build / test process, honor the following two
configuration variables:
TOML11_LANGSPEC_GIT_REPOSITORY
Can be set to override the URL from which the repository is cloned.
This allows using a local mirror, including file:// URLs for working
offline or reducing network traffic.
TOML11_LANGSPEC_SOURCE_DIR
Can be set to configure the location at which the repository is
expected. If it already exists no download will be attempted. This
allows avoiding the additional git-clone(1) altogether and use an
existing directory as-is. This offers two new possibilities:
(1) The same checkout can be reused for building multiple
configurations (e.g. Debug versus Release) saving a little bit of
time and disk space.
(2) Experimental changes can easily be applied to the local source
tree without having them destroyed by the build process.
In order for this flexible location to work, the unit tests which
attempt to read files from the repository had to be adjusted. They now
honor an environment variable TOMLDIR which can be set to point to an
alternate root directory.
All defaults are set such that the previous behavior is maintained.
Instead of introducing the TOMLDIR environment variable, an alternative
solution would have been to set the WORKING_DIRECTORY of the tests to
the TOML11_LANGSPEC_SOURCE_DIR and leave the relative paths in these
tests hard-coded. Alas, some tests also expect that they can /write/
into the current working directory which isn't desirable if it is
potentially pointing outside the build tree. I personally prefer to
mount the source directory read-only and build in a fast tempfs, so this
would e a problem. To be perfectly honest, I don't quite understand why
these tests need to write to the file system in the first place, though.
It seems to me that refactoring them to serialize to a std::ostrstream
instead of a std::ofstream would not only simplify but also speed up the
unit tests and avoid file system problems. But there might have been a
hidden reason why actually using the real file system was considered
necessary for these tests, so I didn't went ahead with that change yet.
This removes one #define from each unit test file and ensures consistency between file and module names. This consistency, was not strictly maintained before. I hope that any discrepancies were unintentional and that a 1:1 mapping is actually what is desired. Since the definition is now done at one single place, it would be easy to apply transformations like removing the 'test_' prefix or replacing '_' with '-' if this should be desired.
This patch consistently changes the inclusion order for unit test files to the following: 1. The header of the unit under test (using <> includes). 2. The unit_test.hpp header (using "" includes). 3. Any additional auxiliary test headers (using "" includes and sorted alphabetically). 4. Additional system headers needed for the test (using <> includes and sorted alphabetically). 5. Conditionally included system headers (using <> includes). Putting the unit under test's header at the very beginning has the advantage of also testing that the header is self-contained. It also makes it very quick to tell what unit is tested in this file.
The 'result' class has unwrap() and unwrap_err() member functions overloaded for const lvalue and rvalue *this to avoid an unnecessarily copying the to-be unwrapped object of its containing object is going to be discarded anyway. Alas, the parse() function toml/parser.hpp file stored the parse result in a local `const` variable so, although the unwrap call would have been the last use of the object in each case, the unnecessary copy would still be made. This patch removes the `const` and adds a std::move() to actually benefit from the already implemented optimization.
ctcmkl
force-pushed
the
several-simple-patches
branch
from
c565859
to
86af1b0
Compare
My recent patch had introduced a conditional use-after-move bug into the test_parse_function_compiles function. This patch fixes that by reworking the entire test case into a compile-time check. In my opinion, we're not loosing anything by not actually executing the code (the result wasn't looked at anyway) and the code becomes much clearer by omitting the argument-preparation fluff.
ctcmkl
force-pushed
the
several-simple-patches
branch
from
86af1b0
to
3f197c3
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Thanks for developing this nice TOML library. We started using it in some of our products and noticed some minor issues which are addressed by this series of patches.
Major effects of these patches:
Please let me know if I should squash some of the relatively small commits together or split this series of patches into multiple pull requests. As far as style is concerned, I've tried to infer and follow the existing practice as much as possible. If there are any written guidelines or automated check, I'm sorry for having failed to find them.