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.
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.
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.
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.
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.
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.
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.
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.
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.
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 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.
The fstream classes are notorious for their non-existent error handling.
This adds a C-style fILE * IO (fopen(), etc.) alternative interface, so
that if a user needs reliable error handling, they can use that, albeit
more inconvenient, but more robust approach.
Instead of static_cast calls that convert int to char, literals of type
char are now used directly with the value encoded via escape sequence.
The benefits are:
- code without static_cast is much more compact and expresses intent
better
- fixed value truncation warning on some compilers (e.g. C4309 on Visual
Studio 2017)
Taking this parameter by const reference forces us to copy it (because
we know we're going to store it). Taking it by r-value reference would
suggest that we _might_ take ownership over it and would also force the
user to make a copy if they wish to retain the original value.
Taking this parameter by value however clearly gives us ownership of its
content without forcing a copy if it's implicit conversion from
`const char*` or explicitly handed over to us by the user via std::move.
