David Seifert [Mon, 2 May 2022 20:30:58 +0000 (22:30 +0200)]
Move unconditional `man_MANS` -> `dist_man_MANS`
* Avoids the repetition in `EXTRA_DIST`, and is the recommended
Automake pattern.
* Some `man_MANS` will have to remain, since Automake has a bug
where `dist_man_MANS` in Automake branches do not get included
in the tarball:
https://lists.gnu.org/archive/html/bug-automake/2008-06/msg00019.html
Note that this does not introduce an extra build dependency in any of the three
build systems:
1. Autotools: these steps are done during construction of the portable source
tarball, during which Python 3 is already required (see autogen.sh).
2. CMake: Python 3 is already required by and used in the top level
CMakeLists.txt.
3. MS Build: Python 3 is already used in version generation in
lib/version/version.vcxproj.
This change is motivated by the goal of removing a dependency on Awk. This
commit removes the last remaining Awk usage in all three build systems, as well
as removing dependencies on `type` and `sort` in the MS Build build system.
This commit removes the generation of the intermediate artifacts color_lib,
color_lib-sort, and color_lib-temp. Instead, we generate only the final
artifact, colortbl.h. This consolidation of logic into Python is part of what
allows the removal of dependencies described above.
Note that this does not introduce an extra build dependency in any of the three
build systems:
1. Autotools: these steps are done during construction of the portable source
tarball, during which Python 3 is already required (see autogen.sh).
2. CMake: Python 3 is already required by and used in the top level
CMakeLists.txt.
3. MS Build: Python 3 is already used in version generation in
lib/version/version.vcxproj.
This change is motivated by the goal of removing a dependency on Awk. Though it
also conveniently side steps some line ending portability issues that were being
worked around with `AWK_OPTIONS`. In Python, this is unnecessary because reading
a file in text mode accounts for both flavors of line endings.
sparse: unconditionally include 'graphviz_exit' from cgraph
This fixes a problem encountered in an upcoming commit, that other code cannot
include both this header and cgraph/exit.h if it is building in standalone mode.
cgraph/exit.h is header-only, so should be fine to use even in standalone mode.
Similar to 89edf3fe0ac0378b7d1297a0ca349a15bade3980, this introduces support for
newer versions of Pango. Without this, strict compilation with -Werror in the
CMake build fails when building against Pango ≥ 1.50. The only LASi font
variants are `NORMAL_VARIANT` and `SMALLCAPS`, so mapping Pango to LASi
equivalents is limited to being a coarse-grained lossy conversion.
This constant was performing two overlapping roles:
1. A flag callers could use to indicate they anticipated calling into sfio
functions with the same object from multiple threads.
2. A flag used by the library itself to indicate whether thread-safety was
available.
To deal with both macros having the same name, sfhdr.h was forcing the value
seen internally to zero. This mechanism has numerous shortcomings:
1. It was possible for a caller to use the external value of `SF_MTSAFE`
(010000) that, thanks to the overriding described above, sfio itself would
fail to understand.
2. Including sfio.h within sfio sources without including sfhdr.h was a latent
foot gun that would result in part of sfio believing the `SF_MTSAFE` flag
_did_ have meaning.
3. sfio functions are not thread-safe, with or without `SF_MTSAFE` defined,
and regardless of what value it has.
To take an example to illustrate 3, `sfopen` does something like double-checked
locking¹ to anticipate another thread modifying `f` in-between its first check
and the point at which it calls `SFMTXSTART`. Except `SFMTXSTART` is not a lock.
There is no synchronization point here, and in fact an optimizing compiler will
simply coalesce both the first and second checks into one. It is not clear to me
whether this code was written when multicore machines were uncommon (though this
design is also unsafe on a unicore machine) or if `SFMTXSTART` is some kind of
placeholder that was meant to one day grow a locking mechanism.
It seems safer and more honest to remove this constant, and with it the
implication that thread-safety is toggle-able. Migrating to C stdio (#1998) will
render all of this moot as it is thread-safe by default.
I do not know which platforms offer stat.h, but it does not appear to be
anything Graphviz currently supports. On all currently supported platforms (even
Windows) this file lives at sys/stat.h which is detected separately.
When sys/stat.h is unavailable, `HAVE_SYS_STAT_H` is not `0`. It is undefined,
which was leading to a malformed expression here. This was exposed by the next
commit.
While it is technically possible to replace the `sfstderr` stream with something
other than stderr or use an SFIO-specific format specifier (e.g. `%!`), nothing
in Graphviz permits this. Thus we know all these instances are actually just
doing basic output to stderr. By replacing them with C stdio, they are more
transparent to the compiler and can be implemented more efficiently.
gvpr: replace use of alternative stack implementation with Graphviz generic one
Similar to previous changes to `gc` in 4e2875fd7376338259dcb3ccc8f029d58bdf22dd,
this replaces some duplicated functionality with the generic Graphviz stack
implementation. This also adds checks for allocation failures that were
previously missing.
This function was unsafe to use in the way described. It relied on semantics
that are not guaranteed under C99. That is, the lifetime extension of a struct
member of an rvalue. This changes under C11 to something that would make this
not problematic. But it is unlikely Graphviz will be able to migrate to C11 in
the foreseeable future as MSVC is lacking C11 support.
Usage of `itos` in this way relies on lifetime extension of a struct member in
an rvalue. While these semantics exist in C11 and C++, they do not in C99. As a
result, this causes undefined behavior.
Usage of `itos` in this way relies on lifetime extension of a struct member in
an rvalue. While these semantics exist in C11 and C++, they do not in C99. As a
result, this causes undefined behavior.
It does not seem worth maintaining this abstraction for a single usage.
Especially when the surrounding code directly calls numerous other ctype.h
functions.
gvpack: manage graph collection as a 'std::vector'
This removes a number of manual allocations as well as awkward passing of the
base pointer and size of this array around. We also drop the `nGraphs` hint, as
it is simpler and more optimal to let `std::vector`’s standard allocation
algorithm handle this.
This removes some manual memory management leaving an easier to read function,
as well as removing a static unfreed buffer that impedes Valgrind and ASan.
When ipsepcola is enabled, `gvpack` transitively links against lib/vpsc which is
partly written in C++. A consequence of this is that building `gvpack` requires
a C++ toolchain and linking against the C++ standard library. With this in mind,
there is no advantage for `gvpack` itself to be written in C instead of C++.
Moving to C++ will allow removing some manual memory management and data
structures.
This commit itself is just a straight rename. Upcoming changes will make the
code more idiomatic C++.
gvpack: mark 'gvplugin_neato_layout_LTX_library' as having C linkage
gvpack.c is currently compiled as C, so this symbol already has C linkage by
default. However an upcoming commit converts it to C++, which then results in
link failures due to this defaulting to C++ linkage. To pre-empt that, we note
its linkage explicitly for now. These guards will be removed in a future commit.
gxl2gv: replace custom 'slist' string stack with generic stack
Similar to previous changes to `gc` in 4e2875fd7376338259dcb3ccc8f029d58bdf22dd,
this replaces some duplicated functionality with the generic Graphviz stack
implementation. Apart from reducing code duplication and complexity, this
removes an unnecessary rounding of allocations. The prior code rounded up
`slist` allocations to `sizeof(void*)`, but `malloc` effectively does this
internally anyway. This also adds checks for allocation failures that were
previously missing.
gxl2gv: replace inline stack implementation with generic API
Similar to previous changes to `gc` in 4e2875fd7376338259dcb3ccc8f029d58bdf22dd,
this replaces some duplicated functionality with the generic Graphviz stack
implementation. This removes the previously hard coded nested subgraph limit of
32. The number of supported subgraphs in now solely limited by available memory.
treat 'X_OK' as an alias of 'R_OK' in the Windows unistd.h shim
This is not technically correct (Windows does not have `X_OK` at all), but this
is an accurate approximation for the ways in which Graphviz uses `X_OK`. This is
another step towards replacing lib/ast/compat_unistd.h.
This will enable further migration away from lib/ast/compat_unistd.h. Note that
the `S_ISDIR` wrapper there appears incorrect, in using a platform-specific
constant for the mask. Its definition of `X_OK` also seems incorrect, as this
value has no documented meaning when calling `_access` on Windows.
projects / graphviz / log