The Lefty tool is deprecated and planned to be removed. 1a4ad00b59cc1e1e9302fcbd1a205495ee59cab5 toggled it to default to disabled in
the Autotools build system. The present change is a preparatory commit for
dropping explicit re-enabling of it in CI, allowing it to no longer be compiled
in CI.
This function attempted to save time by caching the results of a graph cycle
search and then reusing them in a future call. The logic used for this caching
is invalid. It compares graph pointers to see if it is operating on the same
graph as last time, but this can return a false positive if the previous graph
has been deallocated and a new (unrelated) graph has been allocated at the same
address.
This logic would eventually have to be unwound or adjusted in order to support
multithreading in this code too.
It is possible this is the root cause of the issue described in #2162, though I
was never able to reproduce that issue.
The culprit was probably this warning which now is removed:
C:\GitLab-Runner\builds\magjac\graphviz\cmd\lneato\mswin32\lneato.c(46,40): warning C4133: 'function': incompatible types - from 'LPSTR' to 'LPCWSTR' [C:\GitLab-Runner\builds\magjac\graphviz\cmd\lneato\lneato.vcxproj]
LPSTR is a char* and LPCWSTR is a const wchar_t*. The former points to
an array of 8-bit characters, while the latter points to an array of
16-bit characters. See
https://docs.microsoft.com/en-us/windows/win32/learnwin32/working-with-strings.
In order for the argument interpretation to work correctly after this
change, another change was necessary. The type of the argv variable is
LPWSTR which is a wchar_t* so we must use wcscmp and wide-character
literal string to get a correct 16-bit unicode character comparison.
Removes this warning:
C:\GitLab-Runner\builds\magjac\graphviz\cmd\lneato\mswin32\lneato.c(47,36): warning C4133: 'function': incompatible types - from 'LPWSTR' to 'const char *' [C:\GitLab-Runner\builds\magjac\graphviz\cmd\lneato\lneato.vcxproj]
Although not necessary, this commit also renames the lpCmdLine
argument to pCmdLine in order to match the naming in the wWinMain
signature.
This commit also removes the expectation that lneato should fail at
least one time out of 100 in the test_tools test. The actual
workaround for the flakiness will be removed in the next commit in
this series.
Magnus Jacobsson [Sat, 15 Jan 2022 16:38:24 +0000 (17:38 +0100)]
lneato: Windows: correct argument types to WinMain
Fixes these warnings:
C:\GitLab-Runner\builds\magjac\graphviz\cmd\lneato\mswin32\lneato.c(27,28): warning C4028: formal parameter 1 different from declaration [C:\GitLab-Runner\builds\magjac\graphviz\cmd\lneato\lneato.vcxproj]
C:\GitLab-Runner\builds\magjac\graphviz\cmd\lneato\mswin32\lneato.c(27,46): warning C4028: formal parameter 2 different from declaration [C:\GitLab-Runner\builds\magjac\graphviz\cmd\lneato\lneato.vcxproj]
See
https://docs.microsoft.com/en-us/windows/win32/api/winbase/nf-winbase-winmain.
It is unclear to me what the purpose of this local was intended to be.
Furthermore it was being mis-allocated as if it were an array of doubles, not
ints.
API BREAK: use unsigned types for 1-bit fields of 'obj_state_s'
This has little impact (the bit encoding of these fields is identical), but it
suppresses numerous -Wconversion warnings from the compiler, worried that an
implicit conversion from `1` to `-1` was unintended.
Unfortunately we cannot xfail this test with `strict=True` because this test
passes or fails across various different platforms with no discernible pattern
to me. Results from the current commit:
It is not typical that testing paths will contain things like spaces and other
characters that require quoting. But this change goes the extra mile to make
sure these echoed commands are always copy-pastable to re-execute them.
run_c: check return code of compiled program before returning
Every call to `run_c` was immediately checking that the compiled program
succeeded. So by moving the check inside `run_c` itself we can write more
concise test cases.
simplify bitarray API to assume arrays are only resized at construction time
The bit array API was written to allow arbitrary growing or shrinking of an
array after its construction, while maintaining its content. After fully rolling
it out, it turns out Graphviz only ever creates such arrays once at a fixed size
and then uses them until free. Carrying a more generic API than was necessary
had a number of negative consequences:
1. `bitarray_resize` was more complicated than it needed to be, in order to
cope with an arbitrary initial size of the input array.
2. `bitarray_resize` used `realloc;memset` instead of the more efficient
`calloc` because it was assuming the caller needed to maintain the original
content.
3. `bitarray_resize` performed a “loose” allocation with the capacity of the
backing buffer larger than the size of the array itself, assuming that
amortizing allocation costs across repeated resizes was a relevant concern.
Dropping this and making the allocation “tight” not only reduces heap
pressure, but allows dropping the `.capacity` member.
4. `bitarray_array_resize_or_exit` had a more awkward calling convention than
necessary, as exemplified by how it is streamlined in this commit.
As such, this change not only simplifies the code but also reduces memory usage.
projects / graphviz / log