smyrna add_column: use cgraph wrapper for allocation
The lib/cgraph/alloc.h wrappers are similar to the older lib/common/memory.h
wrappers except (1) they are header-only and (2) they live in a directory
(cgraph) that is at the root of the dependency tree. The long term plan is to
replace all use of lib/common/memory.h with lib/cgraph/alloc.h.
d1d1ae30865c14bb7e6a75f8772543c10419e48b seems to have incorrectly assumed
multiplication has a higher precedence than casts. In reality, it is the
opposite, meaning the first parameter to this `MAX` call was always 0. So the
entire expression would evaluate to 10. Empirically this seems to have been fine
since this code has been in use for over a decade with no specific problems
blamed on this area. So lets just abbreviate it into what it evaluates to.
1b847b5abf50d2ab0701523b90a20306d0ee5528 seems to have incorrectly assumed
multiplication has a higher precedence than casts. In reality, it is the
opposite, meaning the first parameter to this `MAX` call was always 0. So the
entire expression would evaluate to 10. Empirically this seems to have been fine
since this code has been in use for over a decade with no specific problems
blamed on this area. So lets just abbreviate it into what it evaluates to.
c193a91a0e9d8565e85bd6953a2c64e971d6ab79 seems to have incorrectly assumed
multiplication has a higher precedence than casts. In reality, it is the
opposite, meaning the first parameter to this `MAX` call was always 0. So the
entire expression would evaluate to 20. Empirically this seems to have been fine
since this code has been in use for over a decade with no specific problems
blamed on this area. So lets just abbreviate it into what it evaluates to.
1b847b5abf50d2ab0701523b90a20306d0ee5528 seems to have incorrectly assumed
multiplication has a higher precedence than casts. In reality, it is the
opposite, meaning the first parameter to this `MAX` call was always 0. So the
entire expression would evaluate to 10. Empirically this seems to have been fine
since this code has been in use for over a decade with no specific problems
blamed on this area. So lets just abbreviate it into what it evaluates to.
1b847b5abf50d2ab0701523b90a20306d0ee5528 seems to have incorrectly assumed
multiplication has a higher precedence than casts. In reality, it is the
opposite, meaning the first parameter to this `MAX` call was always 0. So the
entire expression would evaluate to 10. Empirically this seems to have been fine
since this code has been in use for over a decade with no specific problems
blamed on this area. So lets just abbreviate it into what it evaluates to.
sfdpgen beautify_leaves: use cgraph wrappers for allocation
The lib/cgraph/alloc.h wrappers are similar to the older lib/common/memory.h
wrappers except (1) they are header-only and (2) they live in a directory
(cgraph) that is at the root of the dependency tree. The long term plan is to
replace all use of lib/common/memory.h with lib/cgraph/alloc.h.
sfdpgen oned_optimizer_new: use cgraph wrapper for allocation
The lib/cgraph/alloc.h wrappers are similar to the older lib/common/memory.h
wrappers except (1) they are header-only and (2) they live in a directory
(cgraph) that is at the root of the dependency tree. The long term plan is to
replace all use of lib/common/memory.h with lib/cgraph/alloc.h.
sfdpgen spring_electrical_control_new: use cgraph wrapper for allocation
The lib/cgraph/alloc.h wrappers are similar to the older lib/common/memory.h
wrappers except (1) they are header-only and (2) they live in a directory
(cgraph) that is at the root of the dependency tree. The long term plan is to
replace all use of lib/common/memory.h with lib/cgraph/alloc.h.
sfdpgen beautify_leaves: swap a boolean array for a bit array
This reduces memory pressure by using 1-bit per element instead of 1-byte per
element. Unfortunately this introduces 4 -Wsign-conversion warnings due to
indexing in this function being done with signed variables. But the change still
seems like an improvement on the existing code.
Magnus Jacobsson [Tue, 23 Aug 2022 19:09:44 +0000 (21:09 +0200)]
tests: test_rankdir: remove useless generation of copy
The generator expression will not outlive the scope of
'node_shapes_without_svg_shape' or 'all_node_shapes' which both live
in the global scope, so copying is unnecessary.
For details, see
https://github.com/catchorg/Catch2/blob/devel/docs/generators.md#provided-generators.
gvpr: fix: wrap members needing cleanup in a struct and outline
When seeing this code, the compiler emitted numerous warnings like:
gvpr.c: In function ‘gvpr’:
gvpr.c:929:17: warning: variable ‘prog’ might be clobbered by
‘longjmp’ or ‘vfork’ [-Wclobbered]
parse_prog *prog = 0;
^~~~
What it is trying to tell us is that this code violates the assumptions of
`setjmp`/`longjmp`. Referencing any non-volatile stack-allocated variable after
a `longjmp` is undefined behavior.¹
This change extracts all the members that need to be referenced after `longjmp`
into a wrapper function. This basically puts a function call boundary in a place
such that the code no longer violates assumptions of the C standard. It is a
little awkward that we have to do this by hand, as the transformation is
mechanical and the compiler technically has enough information to do this for
us. But `setjmp`/`longjmp` come from the old world and are low level mechanisms.
If the compiler sees a call to them, it assumes you are doing something unusual
and want to do it exactly as you wrote.
While making this adjustment, some white space is adjusted and some coercion to
boolean is changed to explicit comparisons against `NULL`.
I do not understand why, but after this change the compiler still believes
`nextg` can be clobbered by a `longjmp`. While this is true, `nextg` is not used
after `longjmp`-ing back to `gvpr_core`, so I do not know why the compiler
thinks it needs to warn us about this.
Gitlab: #1801
¹ This restriction stems from how `setjmp`/`longjmp` work on certain platforms.
If a variable is live in a register instead of on the stack, it is not
guaranteed to be restored during a `longjmp`. Hence why the value of such
variables is undefined after a `longjmp`.
gxl2gv endElementHandler: use cgraph wrapper for allocation
The lib/cgraph/alloc.h wrappers are similar to the older lib/common/memory.h
wrappers except (1) they are header-only and (2) they live in a directory
(cgraph) that is at the root of the dependency tree. The long term plan is to
replace all use of lib/common/memory.h with lib/cgraph/alloc.h.
mm2gv SparseMatrix_import_matrix_market: use cgraph wrappers for allocation
The lib/cgraph/alloc.h wrappers are similar to the older lib/common/memory.h
wrappers except (1) they are header-only and (2) they live in a directory
(cgraph) that is at the root of the dependency tree. The long term plan is to
replace all use of lib/common/memory.h with lib/cgraph/alloc.h.
This change also fixes a number of unchecked allocation failures where `malloc`
was being called directly and the result was not checked.
cgraph memresize: fix out-of-bounds write on allocation failure
Callers of this function appear to anticipate the possibility of failure, e.g.
`agrealloc`. But the function itself was attempting to zero newly allocated
memory even if the allocation call failed.
It is not clear to me exactly what “warnings about initialization” were being
suppressed here, but I am assuming something well before C99. In C99, we have a
simpler and more concise way of doing zero initialization.
This caused no ill-effects because defining these within Smyrna is a no-op. All
use of these macros occurs outside cmd/smyrna, in the libraries on which Smyrna
depends. But defining these unused macros was creating unnecessary confusion.
fix 'id' attribute in SVG output omitting input graph id
When using an ID attribute on the input graph,¹ this attribute would only be
propagated to some output elements. In particular, generated `linearGradient`
and `radialGradient` elements in SVGs would be missing the ID.
use 'agxbprint' to round points instead of casting in 'Show_boxes'
This has the advantage of avoiding relying on values fitting in the range of an
int. Going through C stdio this way, the full range of doubles can be preserved.
use agxbbuf for dynamic 'Show_boxes' string construction
We have some recent evidence¹ people are using this debug feature. This change
makes some allocations it does more robust (they now exit on failure) and
abbreviates construction in a stack buffer and then duplication into a heap
buffer into direct construction in a heap buffer.
pathplan Pshortestpath: use longs instead of ints for index variables
Within this function, these indices are sometimes calculated using pointer
arithmetic which has a long (technically `ptrdiff_t`) result. Using a long type
for these variables squashes 2 -Wconversion warnings and reduces the likelihood
of arithmetic overflows.
In future, it is possible we should convert this code to `ptrdiff_t` instead. It
is unclear if this would be an improvement, given interaction with the
surrounding code and cases where these indices are set to literals like `-1`.
Changing to `ptrdiff_t` would also be more invasive than just widening the value
range as done by this change. On most contemporary platforms, `long` and
`ptrdiff_t` are the same type, so perhaps the difference is academic.
sparse: remove always true branch in 'SparseMatrix_sum_repeat_entries'
`what_to_sum` can only take on two values, `SUM_REPEATED_NONE` and
`SUM_REPEATED_ALL`. If it is `SUM_REPEATED_NONE` there is an early exit at the
beginning of this function.
API BREAK: libxdot: use size_t instead of int for stats and polygon line points
This allows exceeding counts of 2³¹ - 1, which is not particularly likely. But
its more important effect is to make arithmetic operations less error prone and
make it impossible by-construction to arrive at a negative count in these fields.
There is some interaction between the stats fields and polygon line points
fields, so doing these two separately would have meant an intermediate state
with lots of casting. It seemed simpler and less error prone to do this all at
once.
There is still casting back to int for the `gvrender_*` functions. Modifying
these would have been too invasive. Surprisingly the call to `gvrender_polyline`
affected in this commit is the only instance of a call to it with a non-literal
point count. But modifying it to take a size_t would still have required
too-invasive changes due to other functions it calls.
For lines that would have been changed anyway, this commit also fixes white
space and tightens variable scoping where possible.
API BREAK: libxdot: use size_t instead of int for op sizes/counts
This allows exceeding operation counts of 2³¹ - 1, which is not particularly
likely. But its more important effect is to make arithmetic operations less
error prone and make it impossible by-construction to arrive at a negative
operation size or count.
Doing this as a single sweeping change seemed less error prone and clearer,
despite how large the resulting diff is.
For lines that would have been changed anyway, this commit also fixes white
space and tightens variable scoping where possible.
projects / graphviz / log