This squashes some warnings on Windows and 32-bit platforms:
generic_list.c(25,28): warning C4244: 'function': conversion from 'uint64_t'
to 'size_t', possible loss of data
generic_list.c(47,56): warning C4244: 'function': conversion from 'uint64_t'
to 'size_t', possible loss of data
generic_list.c(47,66): warning C4244: 'function': conversion from 'uint64_t'
to 'size_t', possible loss of data
This application has no ability to handle allocation failures. For example,
allocation failure during adding an attribute would result in setting
`attr_list` to null which would then go on to crash the program. Lets stop
pretending `prune` handles these cases and just exit on allocation failure.
tvnodes.c: In function ‘populate_data’:
tvnodes.c:195:48: warning: declaration of ‘grid’ shadows a global declaration
[-Wshadow]
195 | static void populate_data(Agraph_t * g, grid * grid)
| ~~~~~~~^~~~
tvnodes.c:32:3: note: shadowed declaration is here
32 | } grid;
| ^~~~
This seems a little overly prescriptive – one `grid` was a type and the other
was a value – but maybe the compiler is concerned that a future introduction of
a `sizeof(grid)` call in `populate_data` would be ambiguous.
By using string views, we can avoid the need to duplicate the graph attribute
value here. This allocation that was previously being lost no longer is made at
all.
smyrna: duplicate names when creating new grid columns
This is a step towards solving a memory leak. Note that this involved rewriting
some string comparisons because the originating pointer is no longer stored in
the column’s `name`. It is not clear to me how the previous code was valid
because, e.g., it was comparing by pointer to the `Visible` constant to detect
when a column was called “Visible”. That is, nothing actually assigns a column
name to the address of `Visible`.
smyrna: squash -Wconversion warnings related to grid column count
This squashes the following compiler warnings:
tvnodes.c: In function ‘update_tree’:
tvnodes.c:294:28: warning: conversion to ‘size_t’ {aka ‘long unsigned
int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
294 | types = gv_calloc(g->count, sizeof(GType));
| ~^~~~~~~
tvnodes.c: In function ‘add_column’:
tvnodes.c:312:43: warning: conversion to ‘size_t’ {aka ‘long unsigned
int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
312 | g->columns = gv_recalloc(g->columns, g->count, g->count + 1,
| ~^~~~~~~
tvnodes.c:312:61: warning: conversion to ‘size_t’ {aka ‘long unsigned
int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
312 | g->columns = gv_recalloc(g->columns, g->count, g->count + 1,
| ~~~~~~~~~^~~
`count` is known non-negative all the time, but cannot be easily converted to a
`size_t` because we need to interact with the GTK APIs that take `int` values.
The last non-trivial call to `_sfpopen` was removed in f552c0dece1435773de62334dee3310d1728e22f. Since then, `proc` members have been
allocated and propagated around with data describing the absence of a subprocess
(`pid == -1`). We can reduce memory overhead and decrease code complexity by
removing these semantically dead code paths.
sfio: use a more portable way of determining page size
This code was using a legacy API, `getpagesize`, to opportunistically find the
system virtual memory page size. This change updates to the newer POSIX
`sysconf` API that is available almost everywhere, and adds a workaround for
Windows. This code now has an accurate understanding of the page size on
platforms that did not have `getpagesize` (of which, Windows was one), rather
than falling back on 8192.
673b9f1a7dbde9c9cc5d9a2a22ee835a08ab40ab tried to work around this in the past.
But there remained a foot gun. If you (possibly transitively) included sfio.h
but did not include sfhdr.h, you could end up facing this error.
Surprising as it may seem, nothing in the code base checks for `SF_ERROR`. So a
cleaner solution that removes the problem in perpetuity is to remove our
`SF_ERROR`. Note that `SF_FLAGS` does not need to be adjusted because `SF_ERROR`
was not a public flag.
CI: remove command line argument parsing from the deploy script
This script is called in exactly one place and always passed the `--verbose`
option. This change removes this only remaining option from the script and
unconditionally applies its effect, for a minor simplification.
CMake: stop trying to install Expat and Getopt on MinGW
MinGW does not need these third-party libraries installed; they are already
available in its ecosystem. This was uncovered through using
`--warn-uninitialized -Werror=dev` with CMake:
CMake Error (dev) at cmd/tools/CMakeLists.txt:460 (install):
uninitialized variable 'GETOPT_RUNTIME_LIBRARIES'
CMake: use legacy 'GLUT_INCLUDE_DIR' instead of 'GLUT_INCLUDE_DIRS'
The newer `GLUT_INCLUDE_DIRS` is only provided in CMake ≥ 3.23. Most users and
most of our CI environments are on a version of CMake prior to this and so get
the following when configuring with `--warn-uninitialized -Werror=dev`:
CMake Error (dev) at cmd/smyrna/CMakeLists.txt:87 (target_include_directories):
uninitialized variable 'GLUT_INCLUDE_DIRS'
b5f4448a3f3a70e14ba7d6990b81506cbda82180 made this closer to correct but still
wrong. As its message mentioned, FindDevIL.cmake is frustratingly inconsistent¹
with respect to the rest of the CMake ecosystem.
Some “extra” CI jobs like the static build have been left on Ubuntu 22.04 under
the assumption that we want to run these on the latest LTS release. But for the
ones affected in this commit, being on a distro with the latest toolchains seems
more desirable.
cdt: use 'size_t' types for counting stats instead of 'int'
This squashes two compiler warnings:
dtstat.c: In function ‘dtstat’:
dtstat.c:60:59: warning: conversion to ‘long unsigned int’ from ‘int’ may
change the sign of the result [-Wsign-conversion]
60 | if(!(Count = malloc((ds->dt_max+1)*sizeof(int))) )
| ^
dtstat.c:74:65: warning: conversion to ‘long unsigned int’ from ‘int’ may
change the sign of the result [-Wsign-conversion]
74 | if(!(Count = malloc((ds->dt_n+1)*sizeof(int))) )
| ^
It also allows these functions to now count beyond 2³¹ - 1.
dtstrhash.c: In function ‘dtstrhash’:
dtstrhash.c:36:18: warning: conversion to ‘unsigned int’ from ‘int’ may change
the sign of the result [-Wsign-conversion]
36 | return (h+n)*DT_PRIME;
| ^
`n` is guaranteed non-negative by the logic preceding this.
cdt: make intent to coerce to unsigned clearer in 'dtstrhash'
This squashes some compiler warnings on CentOS 7:
dtstrhash.c: In function 'dtstrhash':
dtstrhash.c:22:18: warning: conversion to 'unsigned int' from 'int' may change
the sign of the result [-Wsign-conversion]
h = (h + (s[0]<<8) + s[1])*DT_PRIME;
^
dtstrhash.c:28:18: warning: conversion to 'unsigned int' from 'int' may change
the sign of the result [-Wsign-conversion]
h = (h + (s[0]<<8) + s[1])*DT_PRIME;
^
dtstrhash.c:30:18: warning: conversion to 'unsigned int' from 'int' may change
the sign of the result [-Wsign-conversion]
h = (h + (s[0]<<8))*DT_PRIME;
smyrna: avoid inadvertent double promotion in 'renderSelectedNodes'
Squashes two compiler warnings:
topviewfuncs.c: In function ‘renderSelectedNodes’:
topviewfuncs.c:283:41: warning: conversion from ‘double’ to ‘GLfloat’ {aka
‘float’} may change value [-Wfloat-conversion]
283 | glVertex3f(pos.x,pos.y,pos.z+0.001);
| ~~~~~^~~~~~
topviewfuncs.c:285:50: warning: conversion from ‘double’ to ‘float’ may
change value [-Wfloat-conversion]
285 | drawCircle(pos.x,pos.y,nodeSize,pos.z+0.001);
| ~~~~~^~~~~~
smyrna: remove 'line' shadowing in 'load_attr_list'
Squashes a compiler warning:
gui/frmobjectui.c:580:10: warning: declaration of ‘line’ shadows a global
declaration [-Wshadow]
580 | char line[BUFSIZ];
| ^~~~
In file included from ./smyrnadefs.h:27,
from gui/gui.h:13,
from gui/frmobjectui.c:15:
../../lib/glcomp/glutils.h:25:7: note: shadowed declaration is here
25 | } line;
| ^~~~
smyrna: remove 'line' shadowing in 'load_attributes'
Squashes a compiler warning:
gui/gui.c: In function ‘load_attributes’:
gui/gui.c:45:10: warning: declaration of ‘line’ shadows a global declaration
[-Wshadow]
45 | char line[BUFSIZ];
| ^~~~
In file included from ./smyrnadefs.h:27,
from gui/gui.h:13,
from gui/gui.c:14:
../../lib/glcomp/glutils.h:25:7: note: shadowed declaration is here
25 | } line;
| ^~~~
smyrna: use a more appropriate type for 'ComboValuesCount'
Squashes two compiler warnings:
gui/gui.c:94:96: warning: conversion to ‘size_t’ {aka ‘long unsigned
int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
94 | attr[attrcount].ComboValuesCount - 1,
| ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~
gui/gui.c:95:78: warning: conversion to ‘size_t’ {aka ‘long unsigned
int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
95 | attr[attrcount].ComboValuesCount,
| ~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~
fix installing example graphs during CPack; rewrite 'SMYRNA_PATH' discovery
This is effectively two commits in one. But there seems to be no way to separate
these changes without the parts individually failing
tests/test_tools.py::test_tools[smyrna]. Smyrna auxiliary files were being
installed incorrectly prior to this commit, so installing them correctly and
fixing how Smyrna finds them needs to be done in a single change.
---
CMake: fix installing example graphs during CPack
When running `cpack`, instead of staging the Smyrna auxiliary files (share/**)
and the example graphs (graphs/**) in build-local directories, they would be
incorrectly staged into their final install paths. This was quite unexpected as
it would result in errors if you did not have permission to write to the install
directories or deleting+replacing files from a previous Graphviz installation if
you did.
This bug was introduced in 67f88eb86c9966daa4d8c3a6f25aee8cd35a046d as an
accidental side effect of enabling Smyrna in the CMake build system. A close
re-reading of the CPack¹ and `GNUInstallDirs`² docs suggests that absolute paths
should never be used in `install` rules:
`CMAKE_INSTALL_<dir>`
Destination for files of a given type. This value may be passed to the
`DESTINATION` options of `install()` commands for the corresponding file
type. It should typically be a path relative to the installation prefix so
that it can be converted to an absolute path in a relocatable way (see
`CMAKE_INSTALL_FULL_<dir>`). However, an absolute path is also allowed.
`CMAKE_INSTALL_FULL_<dir>`
The absolute path generated from the corresponding `CMAKE_INSTALL_<dir>`
value. If the value is not already an absolute path, an absolute path is
constructed typically by prepending the value of the `CMAKE_INSTALL_PREFIX`
variable. However, there are some special cases as documented below.
This change brings the behavior on Linux in line with how CPack operates on
other platforms – the default value of `CMAKE_INSTALL_DATAROOTDIR` is `share`
which is the hard coded value set on non-Linux platforms.
----
smyrna: rewrite 'SMYRNA_PATH' discovery
This aligns Smyrna on other platforms with how Smyrna on Windows locates its
templates, examples etc. This resolves a problem where a build time path was
being baked into the Smyrna binary, preventing it from being relocatable.
The code for locating our own executable in this commit is adapted from public
domain source.³
This commit also lifts the 1024 character limit on path discovery in the Windows
branch of this logic. It now dynamically expands the target buffer until the
current executable name will fit.
Gitlab: fixes #2232 Reported-by: Magnus Jacobsson <magnus.jacobsson@berotec.se>
¹ https://cmake.org/cmake/help/book/mastering-cmake/chapter/Packaging%20With%20CPack.html
² https://cmake.org/cmake/help/latest/module/GNUInstallDirs.html
³ https://github.com/Smattr/clink/blob/main/clink/src/find_me.c as of commit 8cadfc49a74e429fa69afdc460cb2b0662d81260
tests: only accept 'which' results that are adjacent to 'dot'
As described in the code comment, this addresses a problem where testing would
find binaries from a prior Graphviz installation and then spuriously fail.
This is not ideal. It is easy for an upcoming test writer to forget or be
unaware of this quirk and introduce a new direct call to `shutil.which`. But it
seems the best option we have right now.
After this function was written, the `startswith` helper was introduced for
abbreviating the idiom `strncmp(foo, bar, strlen(bar)) == 0`. We can use it to
more concisely and clearly express the intent of this code.
projects / graphviz / log