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.
ortho: use 'UNUSED' to suppress warnings about debug functions
As an alternative to hiding these functions by guarding them with `DEBUG`, this
allows the compiler to see and parse these functions even when not building in
debug mode. This means we can catch syntax errors that slip into this code even
when it is not being emitted into the final binary.
This has little effect right now as debugging is forced on in this file, but
this may be a nice extra safe guard in future.
projects / graphviz / log