replace NULL-hinted agxbuf usage with zero initialization
This leads to easy to read code and fewer things for the caller to think about.
I think this should probably become the default pattern for agxbuf usage for the
cases where the data you are printing is unpredictable or you are unsure.
cgraph: rename 'agxbuf.dyna' and flip its polarity
On the surface, this change looks a little odd – why care about what bit means
what in an internal field of a data structure? But this has an interesting side
effect. After this change, the zeroed state of an agxbuf is a valid 0-sized
dynamically allocated buffer, the same as the end state after `agxbdisown`. In
other words, C99 zero initialization (`agxbuf xb = {0}`) now defines a
reasonably “default” agxbuf state. This gives you all the benefits of the
previous “I don’t know how long my data will be so I’ll just use `BUFSIZ` as a
hint” approach with less typing and less hard coding. The next commit will roll
this out for some simplification in agxbuf usage.
cgraph agxbput_n: take an early exit when input string is zero
An upcoming change makes it possible to enter this function with `xb->ptr` as
`NULL`. It looks as if there is nothing wrong with this; a zero-sized string
fits in any agxbuf, including an unallocated one. However UBSan educates us that
calling `memcpy` with a `NULL` destination pointer is undefined behavior, even
when the passed size is 0. So this change avoids reaching `memcpy` when we know
the function will be a no-op.
Note that this is sort of fixing a latent bug, in that an agxbuf user could
already cause the scenario described above by `agxbdisown`ing an agxbuf and then
calling `agxbput_n` on it with size 0. However no existing callers do this.
cgraph: when an agxbuf to be expanded has 0 size, expand by 'BUFSIZ'
When calling `agxbmore` on a 0-sized buffer, the new size to expand to would be
chosen based on the length of the current string being appended. This string
often originated from user’s input graph and hence its length was often
user-defined. There is nothing strictly wrong with this, but it is a little
unpredictable and sometimes leads to pathological outcomes, e.g:
agxbputc(&xb, 'a'); // expands 0-sized buffer to 1
agxbputc(&xb, 'b'); // doubles 1-sized buffer to 2
agxbputc(&xb, 'c'); // doubles 2-sized buffer to 4
agxbputc(&xb, 'd');
agxbputc(&xb, 'e'); // doubles 4-sized buffer to 8
…
This is functionally correct, but that is a lot of `realloc` calls just to
accrue 5 bytes.¹ It is more convenient to enlarge our initial allocation to
something matching the default hint when an agxbuf is initialized.
The situation described above has not typically been a concern in the past
because a 0-sized agxbuf is hard to obtain to begin with. You need to either
call `agxbinit` while supplying your own backing storage that also has 0 size
(why would you do this?) or reuse an agxbuf after `agxbdisown` (technically
legal, but no good reason to do this). Neither of these scenarios occur anywhere
in the Graphviz tree to my knowledge. However an upcoming change will make
0-sized agxbufs more common.
¹ Some of these `realloc`s are likely no-ops, as the system allocator generally
does not give you exactly 1 byte when you ask for 1. It will probably give you
more and then when you ask to double to 2, it will tell you your existing
pointer is just fine to keep on using. But the agxbuf code does not know any
of these details and will still repeatedly call into `realloc`.
Lefty, and its dependent tools Dotty and Lneato, have been suspected unused for
a long time. They were scheduled for removal, but we took a cautious and very
slow approach to this given how pervasive Graphviz is and the limited visibility
we have into its usage:
1. 2021-09-25 make Lefty default to disabled in the build system¹
2. 2022-01-16 disable Lefty in published graphviz.org packages²
3. 2022-03-28 remove build system support³
This completes the final step of removing the code from the repository. It is
still retained in the history, should anyone need access to it or need to build
a past version of Graphviz with Lefty.
So far, we have only heard two notable reactions from the downstream world:
1. Lefty disabling was not quite complete on the first attempt.⁴
2. There has been an offer of assistance to maintain Lefty.⁵ No patches have
yet emerged, but if and when they do we could feasibly reverse direction on
this and reinstate Lefty and friends.
Gitlab: closes #219, closes #552, related to #1836, #1854
expr: remove unused parameters from 'Exdisc_t.matchf'
I guess when I committed 260f650085eb25b5b68c81a5960232d025fb6942 it was not
obvious to me that this is entirely part of private internal interfaces and the
callback signature involved is not exposed publicly.
expr: remove unused parameters from 'Exdisc_t.setf'
I guess when I committed 5b4de3922a2f169c2e773ffe85651898aedd9351 it was not
obvious to me that this is entirely part of private internal interfaces and the
callback signature involved is not exposed publicly.
expr: remove unused parameters from 'Exdisc_t.keyf'
I guess when I committed 2e9753d9c9886f5bc013212420528e9a18bc592b it was not
obvious to me that this is entirely part of private internal interfaces and the
callback signature involved is not exposed publicly.
expr: remove unused parameters from 'Exdisc_t.binaryf'
I guess when I committed b0ec7b2eb448a2cb68ffb3751e9e054aecc60c24 it was not
obvious to me that this is entirely part of private internal interfaces and the
callback signature involved is not exposed publicly.
expr: remove unused parameters from 'Exdisc_t.convertf'
I guess when I committed 17479ab6569d40a4778870f712226aa7916f3ca3 it was not
obvious to me that this is entirely part of private internal interfaces and the
callback signature involved is not exposed publicly.
expr: remove further unused parameters from 'Exdisc_t.reff'
I guess when I committed ef3ed2a98a46d0641d936c62fde78e719cef98f8 it was not
obvious to me that this is entirely part of private internal interfaces and the
callback signature involved is not exposed publicly.
expr: replace 'exdump' SFIO buffer with an 'agxbuf'
Unfortunately there is no easy way to stage this. We need to do it all at once,
updating every use of `deparse` and all `exdump`’s internals.
This change improves locality – it is more obvious to both users and the
compiler that the contents of this temporary buffer does not need to be retained
beyond calls to `exerror`. This is a small step towards deprecating SFIO.
Now that `Excc_t` is encapsulated in excc.c and no longer considered an
extensible struct, we can stop playing macro tricks to give it extra fields and
just express these more naturally.
This function invokes `dtwalk` passing the callback `global` to be called with
a state parameter it supplies. It was supplying `cc`, but the callback only uses
a single member of this struct. So we can simplify this code by just passing the
stream the callback writes to, `cc->ccdisc-text`, or equivalently `disc->text`.
ccomps: remove 'N_NEW' that was only used in one place
This brings us one step closer to having a single `N_NEW` macro in the tree and
not having to constantly wonder which of the `N_NEW` definitions is in scope at
any point where you see it called.
Rather than allocating this upfront and then potentially needing to free it
later, we can use a string view to postpone the allocation to when we know it is
actually needed. This works because the lifetime of the backing memory extends
until the call to `g_free(families)`. This also fixes the lack of allocation
failure checks; instead of `strdup` we now call `strview_str` that gracefully
exits on allocation failure.
The type of the fields of `pts` can vary between `float` and `double` depending
compile time options, which is what this code was attempting to deal with. But
`float` and `double` are treated identically with respect to `printf`, so there
is no need to cast.
Here again the proper solution would be to make the struct’s field also a
`size_t` but once again it is part of the public API that we would like to avoid
breaking.
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.
projects / graphviz / log