gv_trim_zeros: identify string extent instead of writing a '\0'
The buffer that this function was truncating is destined for `gvwrite`. So we
can make the whole thing read-only by identifying a string extent instead of
modifying the buffer in place. The compiler may have been able to identify the
intent of this code anyway¹ but if not these changes make it clearer how this
code can be optimized.
This looks like a bit of a strange change, when we now wrap the entire file in
`extern "C"`. However this has two key benefits:
1. `dot_builtins` and `dot_static` that include this source needed an
Autotools hack¹ to force compilation to use the C++ front end (`c++`)
instead of the C front end (`cc`) in order to link against the C++ standard
library. By moving this source into C++ we can remove this hack.
2. When trying to integrate `dot_builtins` into the CMake build system, MSVC
complains (correctly) that the initializers to the array in this file are
not compile-time constants. GCC and Clang apparently allow this by a
non-standard extension. By moving this into C++, we get more relaxed
initialization semantics that allow this on all compilers.
This code is not currently compiled and, in fact, will not compile if you try to
re-enable it. As an example issue, it uses `GD_inleaf`, a macro intended for
accessing `Agraphinfo_t` fields, on a `node_t`. This is sort of a double mistake
as `Agraphinfo_t` also has no `inleaf` field. This problem seems to have been
present in the very first Graphviz revision, 256ef66663ca0c072554ee3f5e7971911031b3c7. Fortunately the mistakes sort of
cancelled each other out because the `GD_*` marcos did no casting and
`Agnodeinfo_t` _does_ have an `inleaf` field. The outcome seems to be what the
author intended, even if the route by which they got there was not intended.
The above is only one of several issues with this code. Resurrecting it has
unknown cost and unknown benefit, so we remove it here to avoid the implication
that it can be easily switched back on.
Contrary to the X11 documentation,¹ it seems button values other than 1-5 can be
returned as button press events. The assertions altered in this commit were
introduced to guarantee the value does not exceed the limits of the type of the
parameter in the user’s callback (`int`). So we can safely relax this to just
the limit itself.
This was validated by doing an exhaustive comparison of all strlen ≤2 inputs to
both the before and after function. Not bulletproof, but it is a strong signal
that the new version is functionally identical.
This function gracefully handled allocation failures by returning 0. Except its
caller does not check for this and assumes the returned pointer is non-null.
This also removes the conditional on `argc` in this function. The contained loop
is a no-op when `argc` is 0 and in this case `argv` is NULL, which it is well
defined to `free`.
fix: Revert "rewrite versionStr2Version to use strtoul"
This reverts commit 5aa56175c4715700cc8bf9fecad7b27665c04379. This commit
inadvertently changed the parsing of the `xdotversion` attribute to make
something like “1.7” parse as version 1.
By construction, `newopn` can only ever be non-negative here. Though in the long
term a more sweeping change should be done to count items in `size_t` variables
pervasively.
reallyroutespline: squash a -Wsign-conversion warning
By construction, `inpn` can only ever be non-negative here. Though in the long
term a more sweeping change should be done to count items in `size_t` variables
pervasively.
The substring construction in `gvplugin_graph` unfortunately cannot be easily
converted to `strview_t` because it passes pointers to `agxset` which expects a
`'\0'` terminated string. We can at least fix previously unnoticed allocation
failures though.
This fixes a memory leak that would occur when duplicate items were encountered.
The conditional block in this loop would not be entered and the memory that `q`
pointed to would be lost in the next iteration of the loop when `q` was set to a
new string.
Similar to the previous commit, this string is either `NULL` or terminated by a
`'\0'`, so there is little advantage in using a `strview_t` over a
`const char*`. But this will make it more consistent with surrounding code after
upcoming changes.
This string is always terminated by `'\0'`, not `':'`, so there is little
advantage in using a `strview_t` over a `const char*`. But upcoming changes will
refactor the surrounding code to use `strview_t`, so it will be more readable to
use `strview_t` consistently here.
2. b4947d67a4ebd48ca0105d44f92e47f044e51600 appears to have applied some
Coverity suggestions without investigating the underlying history that led
to the Coverity warnings.
When calling `merge_chain` from `interclexp`, this code apparently did not
anticipate that both the condition `ND_rank(agtail(e)) == ND_rank(aghead(e))`
and `ED_to_virt(prev) != NULL` could be true at once. In this case, the merge
can happen but the `.to_virt` member of `e` needs to be overwritten; it is not
`NULL` on entry to `merge_chain`.
This issue appears to have existed since the first revision of Graphviz.
These is a lot of uncertainty around what the expected behavior of this code is
and whether this fix is correct. So here is some of my background thoughts:¹
`merge_chain` has a leading `assert(ED_to_virt(e) == NULL)`, presumably trying
to express the assumption that it is not overwriting an existing back pointer.
But the code here in `interclexp` seems to be deliberately trying to overwrite
this pointer. There seemed to me two possible local fixes:
1. Remove the assertion
2. Write `NULL` to `ED_to_virt(e)` in advance
The first possibility seemed more risky. This would allow other functions to
accidentally call into merge_chain with a non-null `ED_to_virt(e)` and for
this to go undetected.
On the other hand, it's possible my change here is mistaken and what the
original author intended was for the true case of the branch on line 181 to
continue after updating `ED_to_virt(e) = prev`, making the call to
`merge_chain` not reached in this scenario. It certainly looks pretty weird
for the code after my changes to set `ED_to_virt(e)` on lines 181-184 and then
subsequently overwrite this value on line 187. But I was guessing the initial
write is relevant for the case where we continue.
As you can tell, all of this is (barely) educated guess work. I find the
intent of this code very hard to determine.
Gitlab: fixes #121
¹ Quoted from discussion on
https://gitlab.com/graphviz/graphviz/-/merge_requests/2724#note_1005393861.
Debian’s /etc/os-release does not include the field `ID_LIKE`, which is perhaps
reasonable as it is a base distribution not derived from another. A side effect
of this was that CPack picked the RPM back end when generating a Graphviz
package. This change teaches it that Debian should also get the DEB back end.
gvplugin_list: remove a second unnecessary duplication of 'typestr'
By managing an extent into the original string, we can avoid a dynamic
allocation in this code. This is similar in spirit to 5a13ab3f08b2066a795ea152f9594f6719d3aaa6.
gvplugin_list: remove unnecessary duplication of 'typestr'
By managing an extent into the original string, we can avoid a dynamic
allocation in this code. This is similar in spirit to 5a13ab3f08b2066a795ea152f9594f6719d3aaa6.
The code here actually looks wrong. It tries to pull out colon-separated
entries, but then uses the full string (`pnext->typestr`) in the call to
`agxbprint`. But this commit leaves this as-is for now.
gvplugin_list: remove unnecessary duplication of 'str'
By managing an extent into the original string, we can avoid a dynamic
allocation in this code. This is similar in spirit to 5a13ab3f08b2066a795ea152f9594f6719d3aaa6.
projects / graphviz / log