pathplan Pobsopen: use a 'size_t' when counting objects
This squashes 3 -Wsign-conversion warnings and is generally closer to what we
would like to do here. The “proper” fix is for fields like `vconfig_t.N` to
become `size_t` instead of `int`. But unfortunately they are part of the public
API, and it seems undesirable to break API for this.
Note that an assumption previously implicit in this function, that all inputs
had a non-negative polygon count, is now an explicit assertion.
The calling code does not rely on the semantics of `malloc` returning `NULL`
when called with a 0 size. It unconditionally frees these arrays in `Pobsclose`
so allocations returning a non-`NULL` pointer for 0-sized allocations (which
Glibc typically does) is fine. Maintaining a wrapper that enforces this is
unnecessary.
This also removes the `method` parameter to this function, which was only ever
set to a single value. This squashes 8 compiler warnings and drops a lot of dead
code.
gvpr canontoken: use char instead of unsigned char, introduce casts
It is not clear to me why this code was using unsigned char when it can do the
same thing with less typing and fewer compiler warnings using char. This also
introduces casts to squash warnings from some of the more pedantic compiler
implementations. See 6c29170f9f29466374fbc6e8e62a1b6916c6bc59 for details.
ast onematch: rephrase hash-based switch into string comparisons
This code was using a hand rolled hash function to implement a series of string
comparisons as a jump table. I guess at some point this must have been a
necessary optimization due to limitations of the day’s compilers/machines. In a
modern environment, this is a deoptimization, impeding the compiler’s ability to
understand your intent. Modern compilers know the string comparison functions as
built-ins and can use SIMD¹/SWAR² tricks to emit a short string comparison as a
single instruction. They are also capable of transforming an if-then-else ladder
into a switch if their heuristics predict it will be worthwhile.
The computation of the return value for this function was relying on string
lengths that fitted in an `int`, something that is generally but not always
true. The compiler complained about this with -Wconversion. The only caller of
this function does not use the return value, so lets just remove it.
This is computing the number of bytes remaining in `buf`; the number of bytes
between a pointer to the current offset and the end of `buf`. Thus it is always
non-negative.
Note that the call to this function from
`constrained_majorization_new_with_gaps` also seems confused about the meaning
of its parameters. But no attempt is made to correct this.
smyrna on_attrAddBtn_clicked: use const type to squash a -Wcast-qual warning
Unfortunately we cannot also do the same for `attr_name` in this function
because it is passed to a Graphviz function that accepts a non-const pointer.
smyrna set_refresh_filters: use const types to squash some -Wcast-qual warnings
Unfortunately we cannot also do the same for `attr_name` in this function
because it is passed to a Graphviz function that accepts a non-const pointer.
Note this also makes `widget` unused in `on_attrApplyBtn_clicked` which is a
Glade callback and cannot have its corresponding parameter removed. So we squash
the resulting warning there.
Magnus Jacobsson [Tue, 12 Jul 2022 13:15:50 +0000 (15:15 +0200)]
remove GD_drawing(g) NULL guard
This guard is unnecessary since `graph_cleanup` alrady has the
necessary guards. This also ensures that the Agraphinfo_t record and
the graph's label is cleaned away even if the graph for some reason
doesn't have a drawing (can this ever happen?).
gvFreeLayout: remove incorrect comment about only the root graph having a drawing
This comment was added in c55b546e3965a8dfd2a66763961c4a8003e433bf,
but both subgraphs and root graphs have a reference to (the same)
drawing through this statement in gvLayoutJobs:
Magnus Jacobsson [Mon, 18 Jul 2022 13:02:59 +0000 (15:02 +0200)]
twopigen: layout: fix heap-buffer-overflow by storing dimension in root graph
The layout allocates memory based on the dimension of the root graph,
but since the dimension was stored in the subgraph and the dimension
in the root graph defaulted to zero, too little memory was allocated.
An alternative solution would have been to use the dimension of the
subgraph, but this had other implications and the chosen solution is
the same as what the other two layout engines supporting the dim
attribute (neato and sfdp) use. Twopi always uses two dimensions.
This fixes the last memory issue detected by ASan in the
test_subgraph_layout test, which now runs without failures.
Magnus Jacobsson [Mon, 18 Jul 2022 12:56:45 +0000 (14:56 +0200)]
circogen: layout: fix heap-buffer-overflow by storing dimension in root graph
The layout allocates memory based on the dimension of the root graph,
but since the dimension was stored in the subgraph and the dimension
in the root graph defaulted to zero, too little memory was allocated.
An alternative solution would have been to use the dimension of the
subgraph, but this had other implications and the chosen solution is
the same as what the other two layout engines supporting the `dim`
attribute (neato and sfdp) use. Circo always uses two dimensions.
projects / graphviz / log