mingle bundle: replace a 'unique_ptr' with a 'vector'
Similar to the prior commit, when the `std::unique_ptr` usage was added here in 176c8a163a951a2b801956ed4a00da05603e3a06, I was incorrectly too focused on
preserving the previous property of this being a heap-allocated array. This is
not necessary, and using a `std::vector` instead allows more flexibility. E.g.
depending on the `std::vector` implementation, it could choose to allocate this
array on the stack instead of the heap.
mingle genBundleColors: replace a 'unique_ptr' with a 'vector'
When the `std::unique_ptr` usage was added here in 869891f9c87b326ddef4de461b92de98ec647b82, I was incorrectly too focused on
preserving the previous property of this being a heap-allocated array. This is
not necessary, and using a `std::vector` instead allows more flexibility. E.g.
depending on the `std::vector` implementation, it could choose to allocate this
array on the stack instead of the heap.
It is fairly clear from the surrounding code that these instances were writing
to the wrong destination and, in some cases, reading from the wrong sources. The
effect of this bug was a little slippery as the underlying storage is a union
with `integer` and `floating` occupying the same storage, so this change does
not alter which bytes are stored to. Additionally users rarely left or right
shift within GVPR scripts, so it is unlikely this affected any real world code.
Despite dredging the commit log, comp.lang, and Stack Overflow, I have been
unable to determine what these comments refer to. They read to me like working
around a register allocator bug. AFAIK every production compiler from the last
decade has been capable of these kind of one-line shifts without work arounds.
1a4ad00b59cc1e1e9302fcbd1a205495ee59cab5 made the Autotools build system default
to disabling Lefty as a step towards its deprecation. As three months have
passed, the present commit takes the next step and removes Lefty support from
the portable tarball and stops building it in CI. Users who still want Lefty can
build from source and re-enable it. If we hear from no such users in the next
three months, build system support for Lefty will be removed altogether. If a
further three months go by with no complaints, Lefty will then be deleted from
the repository.
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.
projects / graphviz / log