When we need to maintain a fallback option anyway, having a vasprintf
implementation does not gain us much. Moreover, nothing in the build system was
checking for the existence of vasprintf, so this code was never being used
anyway.
While this often works out on certain architectures, it is technically not
allowed to use a va_list twice. This change fixes the el function for platforms
where reusing va_lists is not possible.
C99 vsnprintf semantics let you call the function with a NULL pointer to
discover the required number of bytes to print the given string. With this
ability, there is no longer any advantage to having two paths through this
function.
The atoi function has no ability to report failure. There is no advantage to
using it over the safer strto* functions. This change also removes a
-Wconversion warning.
manage _run member of Text as a value instead of a pointer
This member is never set to NULL and the Run class is not involved in any
inheritance hierarchy. So there is no need to heap-allocate it or pass it around
by pointer.
This removes the need to manually manage memory for this member but, more
importantly makes this class copy-safe. The implicit copy constructor of this
class would copy the pointer in the _text member. When either the original
object or the new copy was deleted, the pointer would be freed leaving its
duplicate in the other copy dangling. Any attempt to access this would result in
use-after-free and deleting the object would cause a double-free.
Following this change, it is safe to copy a Run object. This issue was latent,
because no existing code causes a Run object to be copied.
As of the previous commit, Python >3.8 is installed prior to running this
script, so no fallback should be required. Python 3.8 can be removed from the
Windows build utilities repository.¹
install Python 3 as a pre-requisite in Windows CI tasks
Prior to this change, Python 3.8 was used on Windows in CI via the Windows
build dependencies repository.¹ This works but has a number of shortcomings:
1. Upgrading the version of Python used in CI requires access to a Windows
machine; and
2. Upgrading the version of Python used in CI requires building CPython from
source; and
3. Python-dependent failures are hard to root cause because they may be an
issue with how we built CPython.²
As of this commit, Python 3 is installed via the Chocolatey³ package manager.
At time of writing, this installs Python 3.9.4. This is not pinned to a specific
version – i.e. a new Chocolatey package of Python 3 will result in Graphviz
immediately using that in CI. I believe this is actually the behavior we want
right now; pro-actively catching any issues caused by Python upgrades.
This does not change the baseline requirement for Graphviz CI/test scripts from
Python 3.6. Scripts should still be written to be backwards compatible to 3.6.
As this change introduces a new non-trivial step in the Windows CI tasks,
execution time is relevant. At time of writing, the macOS jobs dominate Graphviz
CI execution time, both before and after this change. Thus I think this is
acceptable.
¹ https://gitlab.com/graphviz/graphviz-build-utilities
² E.g. #2000, “CI Python OSErrors from Windows tasks”
³ https://chocolatey.org/
manage events collections in vpsc as values instead of pointers
This removes some manual memory management and optimizes the layout of these
elements (note emplace_back). Memory for individual events is now retained
slightly longer because a manual deletion of pointers event-by-event was removed
and we now rely on the final vector clean up to free this memory. But the impact
of this is expected to be negligible and outweighed by the improved layout.
This is safer and has the potential to be more efficient. Note that this
required rewriting the involved comparator to judge less-than instead of a
three-outcome comparison.
make events a local variable in generateXConstraints and generateYConstraints
Not only is this variable not used outside its containing file (could be
declared static), but its lifetime within both these functions is entirely
contained. Reusing the same global in these functions has no advantage, but
impairs the compiler’s ability to optimize by potentially confusing its alias
analysis.
This removes a layer of indirection. Previously data was being written to the
persistent scratch space, ex->tmp, and then copied to a further temporary buffer
in tok. We now simply allocate to tok and write directly into it. This also
addresses an issue where the persistent buffer was being unconditionally used,
despite that it may have failed to be opened due to lack of memory.
This removes a layer of indirection. Previously data was being written to the
persistent scratch space, ex->tmp, and then copied to a further temporary buffer
in tok. We now simply allocate to tok and write directly into it. This also
addresses an issue where the persistent buffer was being unconditionally used,
despite that it may have failed to be opened due to lack of memory.
use a local buffer for expr sprintf instead of a persistent one
Though this introduces another call to sfstropen, it is actually a step towards
removing sfstropen calls. The first part of this is localizing existing usages;
avoiding the ex->tmp buffer. Then the implementation of expr’s sprintf itself
will need to be refactored to operate on a non-SFIO buffer.
Note that this also addresses an issue where expr’s sprintf would use the
persistent buffer even if prior opening of it had failed due to lack of memory.
Having unraveled the use of longjmp here, the jmp_buf and its setup are no
longer required.
On GCC 8.3.0 this removes a -Wclobbered warning. Though interestingly the
warning text itself is a false positive, referencing nverts that is not used in
the setjmp(…)!=0 path. The actual dangerously used variables in this scenario
were polygon_list and vertex_list. My take away here is that *any* -Wclobbered
warning is typically an indicator of incorrect code. I have never seen a
-Wclobbered warning (from any compiler) for code that was not buggy.
find_ints does not currently return negative values. It returns only 0 or 1.
However, we would like to unravel a use of longjmp within it, so returning a
negative value to indicate an error is convenient. This is preparation for
upcoming changes. Related to #1801.
If a test compiles and/or runs C code and that test fails, included in the test
suite output will now be the commands themselves that were run, echoed in the
style of Bash’s set -x.
projects / graphviz / log