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.
dotgen flat_limits: fix truncation to int during intermediate calculations
Like the prior commits, the problem fixed in this commit seems to have been a
mistake in ebd6191b0eec6e23d96c92aaa06212de339207e3 in not updating these
variables to doubles when transitioning to double-based points. Squashes 3
-Wfloat-conversion warnings.
dotgen make_flat_labeled_edge: fix truncation to int during intermediate ops
Like the prior commits, the problem fixed in this commit seems to have been a
mistake in ebd6191b0eec6e23d96c92aaa06212de339207e3 in not updating these
variables to doubles when transitioning to double-based points. Squashes 1
-Wfloat-conversion warnings.
dotgen make_flat_adj_edges: fix truncation to int during intermediate calculations
Like the prior commits, the problem fixed in this commit seems to have been a
mistake in ebd6191b0eec6e23d96c92aaa06212de339207e3 in not updating these
variables to doubles when transitioning to double-based points. Squashes 4
-Wfloat-conversion warnings.
dotgen _dot_splines: fix truncation to int during intermediate calculations
Like the prior commit, the problem fixed in this commit seems to have been a
mistake in ebd6191b0eec6e23d96c92aaa06212de339207e3 in not updating these
variables to doubles when transitioning to double-based points. Squashes 5
-Wfloat-conversion warnings.
common record_path: fix truncation to int during path calculation
This looks to have been a typo in ebd6191b0eec6e23d96c92aaa06212de339207e3 that
switched this code to operate on double points instead of int points, in that it
did not also update the local temporaries used in this calculation. As a result,
this function produced inaccurate answers due to rounding when points were too
close together and downright wrong answers when points did not fit in an int
data type. Squashes 4 -Wfloat-conversion warnings.
common point_gencode: tighter scope and stack-allocate 'AF'
`AF` was being dynamically allocated as if it needed to contain an arbitrary
(potentially large) number of points. However its only use is in being passed to
`gvrender_ellipse` which only looks at the first two points. So we can simplify
and optimize this code by simply stack-allocating 2 entries.
common emit_html_rules: perform base calculation uniformly on doubles
It is unclear to me why 070d24215a27219b33581c96c39e6e8811ba52a7 chose to
perform these intermediate calculations truncating to unsigned char. This
resulted in loss of accuracy and multiple possibilities for overflow. This
change makes the computation more natural, squashing 7 compiler warnings.
The unusual structure of the loop in this function made it appear as if it was
intending to cope with e.g. an `n` of 0 but still print the first element of
`p`. Surveying the callers of `gvprintpointflist`, we can see it is never used
this way. So we can write it simpler and more readably.
The `$GV_FILE_PATH` environment variable could be set to sandbox Graphviz’
ability to read and write to the file system. This made sense once upon a time,
but the world around Graphviz has shifted. Sandboxing yourself is no longer as
valuable a proposition as an external sandboxer that can be more easily audited.
Platforms’ ecosystems have matured to support this use case (Capsicum on
FreeBSD, Seccomp on Linux, App Sandbox on macOS, Pledge on OpenBSD, …).
This change makes any attempt to use `$GV_FILE_PATH` “fail-closed,” in the
sense that execution will be aborted. This may be surprising and not what the
user intended, but this conservatively guarantees safety: you can never think
you have enabled `$GV_FILE_PATH`-based sandboxing and be instead running
unguarded.
1d28d7d2b4d2b2551bd1f432aa175f54a69364a4 appears to have copy-pasted much of the
initial code from cmd/tools/cvtgxl.c, accidentally retaining this usage message
referring to the wrong tool.
This makes very little difference to the compiler, as it can see all uses of
these strings. But this makes it clearer to readers that these variables are not
modified. Using an array instead of a pointer slightly increases flexibility,
allowing a useful application of `sizeof` to these variables should we ever want
to do that.
DEVELOPERS.md: move how to make a release to the bottom
This guide was sort of back-to-front, in that it prioritized readers who were
maintainers looking at cutting an upcoming release. In reality, this document is
read far more often by drive-by contributors seeking how to build and test their
changes.
DEVELOPERS.md: drop TODO about updating example commits
Despite coming up on 5.0.1, we have not updated the example commits since
2.44.1. This seems like strong evidence that the existing examples are
sufficient.
In a sort of Chesterton’s fence¹ sense, I do not understand the existing code.
While I understand the first order motivation (dynamic string buffering inline),
I do not follow the implementation. It manages to hit the trifecta:
1. NULL pointer dereference. Failures of allocation functions were not
checked.
2. Memory leaks. Calls to `agstrdup` and `agstrdup_html` allocate an owned
string internally, yet this code was acting as if they stole the `Sbuf`
backing memory.
3. Not-obviously-correct and repeated computation of the end of the buffer to
append to. `addstr` seemed overly complex and inefficient.
Static initialization of an agxbuf was a bit complicated until 04a248348798a4a32fc9ac7e5d860a84ab5ee49f, though it was still possible.
Following that commit, it becomes a no-brainer.
The code after this change still leaks memory. Perhaps in future we should reset
(`agxbfree(&Sbuf); memset(&Sbuf, 0, sizeof(Sbuf));`) the buffer after `agstrdup`
or `agstrdup_html`, though there would be a performance cost to this.
projects / graphviz / log