gvpr mkBlock: more precise bounds on internal buffers
On most platforms, `BUFSIZ` is likely large enough to contain the data printed
to it. But since we know the exact data that is placed in these buffers, we can
write this code more portably.
Elsewhere in Graphviz we sometimes need to print ints into fixed sized buffers,
but not using itos. So it is useful to have a value for the number of bytes this
requires that is also regularly validated by the Graphviz test suite. That is,
uses of this constant should be robust to future environment/platform changes,
because the itos tests will fail, prompting an update of this constant, that
will then correctly affect all usages of this.
Now that the backing memory for `endgsfx` is no longer hosted in a temporary
sfio buffer that may be overwritten, there is no need to duplicate this further.
teach the Autotools build system that PHP headers are not part of Graphviz
tl;dr: this removes 166 spurious compiler warnings.
When building the Graphviz PHP bindings, PHP headers are included and the build
system was treating these as local includes; things to be warned about. With PHP
7, this was generating a *lot* of warnings. Many of these issues are already
fixed upstream in PHP but, moreover, these files are not part of Graphviz and
warnings about them should not be part of the Graphviz build.
This commit teaches the compiler that the PHP headers are system headers, not
local headers, and warnings about them should be suppressed.
Some open questions:
1. I explored finding an alternative `--includes` option to `php-config` that
would give `-isystem` prefixed paths instead of `-I` prefixed paths. It
would be cleaner than this commit to do such a thing in configure.ac, but
such an option does not appear to exist.
2. Vim highlights the use of `$(patsubst …)` as if it is an error. Is it
illegal to use GNU Make functions in Makefile.am files? I do not think so,
but given how long both Autotools and Vim have co-existed for, I am
wondering if this highlighting is deliberate and telling me something I am
missing.
Magnus Jacobsson [Wed, 15 Sep 2021 17:10:52 +0000 (19:10 +0200)]
CI: remove obsolete exporting of SHELLOPTS = "igncr" in windows-cygwin-cmake-build
This is no longer necessary since crlf is now replaced with lf in all
text files in the repository before Cygwin starts.
Exporting SHELLOPTS has the negative side effect that other flags such
as -e, -u and -x are inherited by child processes which could cause
invoked bash scripts to fail (because of -u) and abort (because of -e)
unintentionally. While this does not happen in the current Cygwin
CMake build, this has been observed in Cygwin autotools builds that
will be introduced in an upcoming MR. This commit ensures that this
can not occur in the future for CMake builds either.
The -x option is more or less harmless, but causes excessive logging
in invoked scripts. This can be seen in
e.g. https://gitlab.com/graphviz/graphviz/-/jobs/1593005342.
smyrna update_columns: fix incorrect call to initGrid
Thanks to backwards compatibility with K&R C, a function declared with no
parameter list takes an unspecified number of parameters. So it is legal to call
such a function with any arguments. Nevertheless on macOS Clang notices:
tvnodes.c:354:18: warning: too many arguments in call to 'initGrid'
as noted by Stephen.¹ This commit both fixes the call and alters `initGrid` to
have a more strict signature. This appears to be the correct fix to me as `str`
is duped into `g->flds` below this which is what I think the mistaken call to
`initGrid` believed was happening inside `initGrid`.
smyrna object_color: rephrase to make initialization of 'vis' more obvious
Auditing all call sites of this function reveals it is only ever called with
edges and nodes. But the compiler could not see enough to know this. On macOS,
Clang dutifully warns:
topviewfuncs.c:165:13: warning: variable 'vis' is used uninitialized whenever
'if' condition is false [-Wsometimes-uninitialized]
This code was using bitwise operations to substitute for logical boolean
operators. This is sometimes justified, to avoid the short-circuit behavior of
the logical operators that can impair CPU pre-fetching/speculation. However, in
this situation this code is not on a hot path and the bitwise operators were
just making this code harder to read.
fix: do not overwrite exparse.h during MSBuild compilation
Commit 09973560cc67c5277df8732ebba75da784afbe51 altered exparse.h to be a static
checked-in file instead of generated by Bison. This passed CI testing, but
unfortunately it was not noticed that on Windows MSBuild this resulted in the
build process still generating exparse.h and overwriting the in-repo copy.
This change reverts the Bison command used during MSBuild to generate the
default y.tab.c/y.tab.h output, more closely aligning it with the other build
systems.
On most platforms, man has built in support for reading gzipped man pages so it
is an advantage to compress these at build time to reduce Graphviz’ on-disk
footprint.
In commit 7d0a6af5edd4cd1408f04548bc0e49cf8dfa5ac2 that added this code, it is
not clear what the purpose of this was. Compiler warnings and more-debuggable
compiler outputs are completely orthogonal things. Now configure.ac
unconditionally enables `-Wall` earlier so this code is redundant.
stop attempting to suppress double '-O?' with --enable-debug
Similar to the prior commit, there is little advantage to these error prone
gymnastics when the compiler is perfectly happy to let a later option supersede
an earlier one. Note that for compilers that are not GCC or ICC, this means
there optimization flags are no longer altered when passing `--enable-debug`.
stop attempting to suppress double '-g' with --enable-debug
This reverts part of 3b7da5bc0345872efcf011c9c614cc27f82b9998. It is unclear
whether this was a typo or just an insufficiently scoped attempt to clean up
build flags. Of the problems with this code, the main ones were:
1. It seems somewhat purposeless to suppress `-g` only to just re-add it. GCC
and ICC can both cope with duplicate `-g` options. It seems purely
aesthetically-motivated gymnastics to try and pass the compiler a sole
`-g`.
2. The scope of things that matched this replacement was too wide. For
example, a `-ggdb` option would be mangled into `gdb` which is almost
certainly not what the user intended and would result in cryptic build
errors. Something like `-fprofile-generate` would be mangled into
`-fprofileenerate` resulting in similarly confusing build errors.
gvtest.py: fix: use os.remove instead of pathlib.unlink to remove a file
There were two problems with using pathlib.unlink here:
1. If the caller did not pass in a value for `dst`, it was assigned to the
second return value of a `tempfile.mkstemp` call. That is, `dst` would be a
string, not a `pathlib.Path`, and calling `.unlink()` would fail
cryptically.
2. The `missing_ok` parameter which this code was using was only added to
`pathlib.unlink` in Python 3.8. The stated Python baseline for Graphviz
currently is Python 3.6. If this code was reached with Python < 3.8 it
would result in an exception, obscuring the original problem.
fix: re-align dotlex’s 'yyerror' with that expected by Bison
Bison 3.8 now emits a prototype for `yyerror`:¹
To comply with the latest POSIX standard, in Yacc compatibility mode
(options `-y`/`--yacc`) Bison now generates prototypes for yyerror and
yylex. In some situations, this is breaking compatibility: if the user
has already declared these functions but with some differences (e.g., to
declare them as static, or to use specific attributes), the generated
parser will fail to compile. To disable these prototypes, #define yyerror
(to `yyerror`), and likewise for yylex.
Because the generated prototype takes a const parameter,² this prototype
conflicts with the implementation in dotlex.c causing a compilation error.
Rather than the work around suggested by the Bison notes, this change simply
re-aligns `yyerror` with the Bison default.
This function does not modify the pointed to data of its argument. However, the
immediate problem this change is addressing is that Bison 3.8 now emits a
prototype for this function:¹
To comply with the latest POSIX standard, in Yacc compatibility mode
(options `-y`/`--yacc`) Bison now generates prototypes for yyerror and
yylex. In some situations, this is breaking compatibility: if the user
has already declared these functions but with some differences (e.g., to
declare them as static, or to use specific attributes), the generated
parser will fail to compile. To disable these prototypes, #define yyerror
(to `yyerror`), and likewise for yylex.
Because the generated prototype takes a const parameter,² these two prototypes
now conflict causing a compilation error. Rather than the work around suggested
by the Bison notes, this change simply re-aligns `gmlerror` with the Bison
default.
This function does not modify the pointed to data of its argument. However, the
immediate problem this change is addressing is that Bison 3.8 now emits a
prototype for this function:¹
To comply with the latest POSIX standard, in Yacc compatibility mode
(options `-y`/`--yacc`) Bison now generates prototypes for yyerror and
yylex. In some situations, this is breaking compatibility: if the user
has already declared these functions but with some differences (e.g., to
declare them as static, or to use specific attributes), the generated
parser will fail to compile. To disable these prototypes, #define yyerror
(to `yyerror`), and likewise for yylex.
Because the generated prototype takes a const parameter,² these two prototypes
now conflict causing a compilation error. Rather than the work around suggested
by the Bison notes, this change simply re-aligns `aagerror` with the Bison
default.
fix: alter expr’s parser’s prefix and introduce a correct yyerror analogue
Bison documents the `yyerror` function that is expected to be supplied by the
user as taking a single string argument.¹ However expr’s parser was redirecting
this to `exerror`, a variadic function. This mostly worked out fine, but only
coincidentally due to calling conventions aligning.
In Bison 3.8, prototypes for `yyerror` are now generated:²
To comply with the latest POSIX standard, in Yacc compatibility mode
(options `-y`/`--yacc`) Bison now generates prototypes for yyerror and
yylex. In some situations, this is breaking compatibility: if the user
has already declared these functions but with some differences (e.g., to
declare them as static, or to use specific attributes), the generated
parser will fail to compile. To disable these prototypes, #define yyerror
(to `yyerror`), and likewise for yylex.
This causes compilation failures due to type signature mismatches between the
Bison-generated prototype and the hand written `exerror` prototype. Rather than
apply Bison’s suggested work around, this commit instead changes the prefix used
for this parser and introduces a short trampoline, `ex_error` to handle
differences between the calling conventions. Not only does this fix compilation
errors, but it also makes this code robust against Bison issuing error messages
containing formatting codes (e.g. "%s") which `exerror` would previously have
misinterpreted as directives to unpack further arguments.
Note that this is backwards compatible. That is, the code following these
changes still works with Bison 3.7 series.
This is the equivalent of 80ccebe66e71c6da570ea5a27f62c21d8c8e39aa but applied
to the install script. Every Linux distro Graphviz CI now runs on appears to
have an /etc/os-release, so this code is unused.
Revert "ensure ID_LIKE is always set on Debian in CI"
This reverts the remainder of commit 029acf11d1d9a9eb20eddae84cadb61447da95f3.
There are currently no configured Linux CI jobs that run outside of Docker
containers, so this work around is unnecessary.
projects / graphviz / log