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.
Graphviz periodically runs into problems where CI test jobs exceed the maximum
timeout and recently the project as a whole has exceeded its Gitlab CI quota.
`test_2095` is by far the longest running component of the test suite. This
change is the result of applying two test case minimizers, one clever¹ and one
not so clever², to the input to this test case. The minimizers were asked to
find smaller input that still (1) provoked a segfault on 588096bd638543ea851ea22751ed91549f61a407 and (2) could be processed successfully
on 32feee561394530713292f8873020fc5feacb9fb. The result takes a ~103KB test
input to ~5KB, with execution time in an example environment dropping from
~155s to <1s.
This dictionary was only ever inserted to by code that was removed in the
previous commit as unreachable. So we know it is always empty and lookups into
it would have always returned null.
graphml2gv: remove dead code handling ending of 'attr' attribute
The code in the last branch in `endElementHandler` was dealing with a closing
`</attr>` tag in the input GraphML. However, the preceding `startElementHandler`
callback rejects an opening `<attr>` tag. So there is no way to reach this
closing logic. This change removes not only the local unreachable code but
several transitively unreachable functions.
As an aside, this was only discovered while trying to construct a test case to
provoke #2300. One path to `dict_relabel` is through `graphml2gv`, by handling a
closing `</attr>` tag. So I looked online for sample GraphML using `attr`. I
could not find any. So I went to the GraphML specification¹ and there discovered
`attr` is not a valid GraphML tag. So I thought, fine, I will figure out how
`graphml2gv` expects this non-standard tag to appear. And that is when I
discovered none of this logic is reachable.
This does not appear to be the result of changes. The very first revision of
`graphml2gv`, 1d28d7d2b4d2b2551bd1f432aa175f54a69364a4, seems to have this
limitation already. I can only conclude this was copy-pasted from gxl2gv without
taking into account the differences between GXL and GraphML.
Boost 1.74.0 deprecates some functions that svgpp is using. Without squashing
these, the CMake build fails in CI:
[ 86%] Building CXX object
tests/CMakeFiles/test_common.dir/svgpp_context.cpp.o
In file included from /usr/include/boost/config/header_deprecated.hpp:18,
from /usr/include/boost/detail/scoped_enum_emulation.hpp:15,
from /usr/include/svgpp/detail/namespace.hpp:13,
from /usr/include/svgpp/detail/attribute_name_to_id.hpp:11,
from
/usr/include/svgpp/attribute_traversal/prioritized.hpp:12,
from
/usr/include/svgpp/attribute_traversal/attribute_traversal.hpp:10,
from /usr/include/svgpp/document_traversal.hpp:10,
from /usr/include/svgpp/svgpp.hpp:1,
from /builds/graphviz/graphviz/tests/svgpp_context.h:7,
from /builds/graphviz/graphviz/tests/svgpp_context.cpp:8:
/usr/include/boost/detail/scoped_enum_emulation.hpp:17:1: note: '#pragma
message: This header is deprecated. Use <boost/core/scoped_enum.hpp>
instead.'
17 | BOOST_HEADER_DEPRECATED("<boost/core/scoped_enum.hpp>")
| ^~~~~~~~~~~~~~~~~~~~~~~
The default toolchain on Ubuntu 22.10 comes with a slightly more pedantic header
layout:
[ 87%] Building CXX object tests/CMakeFiles/test_common.dir/svg_element.cpp.o
tests/svg_element.cpp: In member function 'SVG::SVGRect
SVG::SVGElement::outline_bbox(bool)':
tests/svg_element.cpp:287:29: error: 'all_of' is not a member of 'std'
287 | auto is_vertical = std::all_of(
| ^~~~~~
tests/svg_element.cpp:290:31: error: 'all_of' is not a member of 'std'
290 | auto is_horizontal = std::all_of(
| ^~~~~~
remove 'pointfof' and replace with aggregate initialization
Now that Graphviz is compiled with C99, there does not seem to be much advantage
in retaining this helper function. We have initialization syntax that is the
same number of characters and can be understood locally without having to lookup
the definition of `pointfof`.
unconditionally use 'deflateBound' when zlib is enabled
When Graphviz was built with support for zlib-based compression, it would only
use `deflateBound` if it was discovered at build time. The `deflateBound` code
path provides a tighter estimate of final compressed size, allowing compression
to minimize its intermediate heap allocations.
The CMake build system did not attempt to discover the presence of
`deflateBound` (config-cmake.in). The Windows build hard coded `deflateBound` as
unavailable (windows/include/config.h). So both these builds would result in a
Graphviz that would use more memory than necessary when writing compressed
output formats.
`deflateBound` arrived in zlib 1.2.0, released on 2003-03-09. It seems safe to
unconditionally assume its existence in 2022. The estimate `deflateBound`
provides has improved in successive releases, with even the latest zlib 1.2.13
released on 2022-10-13 further tightening its estimate. So it is expected that
Graphviz users, with this change, will get lower memory usage during writing
compressed formats, and then lower still as newer zlib releases propagate
through the ecosystem.
Calls to this function are generated by Flex – it normally generates calls to
`yyerror` but we alter the Flex prefix to `aag` – but it does not generate a
prototype for the function. So squash the compiler’s overly cautious warning:
../../lib/cgraph/scan.l: At top level:
../../lib/cgraph/scan.l:219:6: warning: no previous prototype for ‘aagerror’
[-Wmissing-prototypes]
219 | void aagerror(const char *str)
| ^~~~~~~~
../../lib/cgraph/scan.l: In function ‘aagerror’:
../../lib/cgraph/scan.l:232:14: warning: switch missing default case
[-Wswitch-default]
232 | else switch (YYSTATE) {
| ^~~~~~
This switch is adding more information to the error message based on which
lexing state we are in, of which four are defined: `INITIAL` (the built-in start
state), `comment`, `hstring`, and `qstring`. In the `INITIAL` state there is no
extra information to add.
cgraph storeFileName: squash warnings when dealing with buffer lengths
This removes the following compiler warnings which were particularly problematic
as they were pointing to the wrong source lines. It is unclear whether this is
due to a Flex bug or the way we are calling Flex.
../../lib/cgraph/scan.l: In function ‘storeFileName’:
../../lib/cgraph/scan.l:98:28: warning: conversion to ‘size_t’ {aka ‘long
unsigned int’} from ‘int’ may change the sign of the result
[-Wsign-conversion]
98 | static char* buf;
| ^
../../lib/cgraph/scan.l:98:37: warning: conversion to ‘size_t’ {aka ‘long
unsigned int’} from ‘int’ may change the sign of the result
[-Wsign-conversion]
98 | static char* buf;
| ^
../../lib/cgraph/scan.l: In function ‘ppDirective’:
../../lib/cgraph/scan.l:125:29: warning: conversion from ‘long int’ to ‘int’
may change value [-Wconversion]
125 | while (*e && *e != '"') e++;
| ~^~
greg [Fri, 7 Oct 2022 00:34:47 +0000 (17:34 -0700)]
Increase stack size for dot MSBUILD project file (avoid crash on larger graphs)
Comment from Matthew Fernandez: It is believed this resolves some instances of
stack overflow in `dfs_enter_outedge` experienced with larger graphs on Windows.
Co-authored-by: Matthew Fernandez <matthew.fernandez@gmail.com>
projects / graphviz / log