str_mpy: [nfc] pre-compute and allocate the result string
This change does not affect the functionality of this function, but it has two
motivating advantages:
1. The temporary scratch buffer `ex->tmp` is no longer used. Though it is not
obvious without auditing a lot of surrounding code, the data written into
this buffer does not need to be retained beyond the lifetime of this
function. Removing its use not only removes a code path through sfio, but
decouples this code from other code using `ex->tmp` making it easier to
understand. Related to #1873, #1998.
2. The prior code used an sfio temporary buffer to construct the result string
and then duplicated it into a vmalloc-allocated buffer. This is reasonable
as vmalloc has no support for incrementally constructing dynamically
allocated strings. However we can avoid the intermediate sfio buffer by
simply pre-computing the final vmalloc allocation that will be needed. This
change does exactly that and simply writes the result once into its final
destination instead of copying through an intermediate buffer. This
should not only (slightly) decrease transient heap pressure, but also
(again slightly) accelerate the performance of this function.
Both these effects are a simplification with respect to how the compiler sees
this function. That is, an optimizing compiler should now better comprehend the
intent of this function and be able to more aggressively specialize and inline
it where relevant.
Upcoming changes will improve the efficiency of this function and decrease its
coupling with other operations. Rather than introduce these new changes in a
differing style, this preparatory commit rewrites the existing functionality in
this style first, without affecting its behavior. Related to #1873, #1998.
str_mod: [nfc] pre-compute and allocate the result string
This change does not affect the functionality of this function, but it has two
motivating advantages:
1. The temporary scratch buffer `ex->tmp` is no longer used. Though it is not
obvious without auditing a lot of surrounding code, the data written into
this buffer does not need to be retained beyond the lifetime of this
function. Removing its use not only removes a code path through sfio, but
decouples this code from other code using `ex->tmp` making it easier to
understand. Related to #1873, #1998.
2. The prior code used an sfio temporary buffer to construct the result string
and then duplicated it into a vmalloc-allocated buffer. This is reasonable
as vmalloc has no support for incrementally constructing dynamically
allocated strings. However we can avoid the intermediate sfio buffer by
simply pre-computing the final vmalloc allocation that will be needed. This
change does exactly that and simply writes the result once into its final
destination instead of copying through an intermediate buffer. This
should not only (slightly) decrease transient heap pressure, but also
(again slightly) accelerate the performance of this function.
Both these effects are a simplification with respect to how the compiler sees
this function. That is, an optimizing compiler should now better comprehend the
intent of this function and be able to more aggressively specialize and inline
it where relevant.
Upcoming changes will improve the efficiency of this function and decrease its
coupling with other operations. Rather than introduce these new changes in a
differing style, this preparatory commit rewrites the existing functionality in
this style first, without affecting its behavior. Related to #1873, #1998.
str_xor: [nfc] pre-compute and allocate the result string
This change does not affect the functionality of this function, but it has two
motivating advantages:
1. The temporary scratch buffer `ex->tmp` is no longer used. Though it is not
obvious without auditing a lot of surrounding code, the data written into
this buffer does not need to be retained beyond the lifetime of this
function. Removing its use not only removes a code path through sfio, but
decouples this code from other code using `ex->tmp` making it easier to
understand. Related to #1873, #1998.
2. The prior code used an sfio temporary buffer to construct the result string
and then duplicated it into a vmalloc-allocated buffer. This is reasonable
as vmalloc has no support for incrementally constructing dynamically
allocated strings. However we can avoid the intermediate sfio buffer by
simply pre-computing the final vmalloc allocation that will be needed. This
change does exactly that and simply writes the result once into its final
destination instead of copying through an intermediate buffer. This
should not only (slightly) decrease transient heap pressure, but also
(again slightly) accelerate the performance of this function.
Both these effects are a simplification with respect to how the compiler sees
this function. That is, an optimizing compiler should now better comprehend the
intent of this function and be able to more aggressively specialize and inline
it where relevant.
Upcoming changes will improve the efficiency of this function and decrease its
coupling with other operations. Rather than introduce these new changes in a
differing style, this preparatory commit rewrites the existing functionality in
this style first, without affecting its behavior. Related to #1873, #1998.
str_and: [nfc] pre-compute and allocate the result string
This change does not affect the functionality of this function, but it has two
motivating advantages:
1. The temporary scratch buffer `ex->tmp` is no longer used. Though it is not
obvious without auditing a lot of surrounding code, the data written into
this buffer does not need to be retained beyond the lifetime of this
function. Removing its use not only removes a code path through sfio, but
decouples this code from other code using `ex->tmp` making it easier to
understand. Related to #1873, #1998.
2. The prior code used an sfio temporary buffer to construct the result string
and then duplicated it into a vmalloc-allocated buffer. This is reasonable
as vmalloc has no support for incrementally constructing dynamically
allocated strings. However we can avoid the intermediate sfio buffer by
simply pre-computing the final vmalloc allocation that will be needed. This
change does exactly that and simply writes the result once into its final
destination instead of copying through an intermediate buffer. This
should not only (slightly) decrease transient heap pressure, but also
(again slightly) accelerate the performance of this function.
Both these effects are a simplification with respect to how the compiler sees
this function. That is, an optimizing compiler should now better comprehend the
intent of this function and be able to more aggressively specialize and inline
it where relevant.
Upcoming changes will improve the efficiency of this function and decrease its
coupling with other operations. Rather than introduce these new changes in a
differing style, this preparatory commit rewrites the existing functionality in
this style first, without affecting its behavior. Related to #1873, #1998.
str_ior: [nfc] pre-compute and allocate the result string
This change does not affect the functionality of this function, but it has two
motivating advantages:
1. The temporary scratch buffer `ex->tmp` is no longer used. Though it is not
obvious without auditing a lot of surrounding code, the data written into
this buffer does not need to be retained beyond the lifetime of this
function. Removing its use not only removes a code path through sfio, but
decouples this code from other code using `ex->tmp` making it easier to
understand. Related to #1873, #1998.
2. The prior code used an sfio temporary buffer to construct the result string
and then duplicated it into a vmalloc-allocated buffer. This is reasonable
as vmalloc has no support for incrementally constructing dynamically
allocated strings. However we can avoid the intermediate sfio buffer by
simply pre-computing the final vmalloc allocation that will be needed. This
change does exactly that and simply writes the result once into its final
destination instead of copying through an intermediate buffer. This
should not only (slightly) decrease transient heap pressure, but also
(again slightly) accelerate the performance of this function.
Both these effects are a simplification with respect to how the compiler sees
this function. That is, an optimizing compiler should now better comprehend the
intent of this function and be able to more aggressively specialize and inline
it where relevant.
Upcoming changes will improve the efficiency of this function and decrease its
coupling with other operations. Rather than introduce these new changes in a
differing style, this preparatory commit rewrites the existing functionality in
this style first, without affecting its behavior. Related to #1873, #1998.
agcallbacks: force flag to be treated as a boolean
We cannot easily change this function’s signature to take a C99 bool without
breaking API, but we can at least ensure it is treating its flag as a boolean
internally. This squashes two -Wconversion warnings.
Now that the switched-on type is represented as an enum, it is simpler to audit
all call sites and confirm that the default case of all these switches is
unreachable.
Nothing was relying on the values of these constants and it is clearer to use
an enum here. It is now more obvious to readers when one of these values is
being passed between functions instead of an ambiguous int.
remove vmalloc indirection through function pointers
In the past, vmalloc supported a configurable allocation method. That is, it was
parametrized with the underlying allocator. Commit 099964281cfccd7b4b0dbc4473ad9347ed3ca2f0 removed support for this and made the
so-called “bestalloc” the only option, directly using malloc. Since then, the
Vmethod_t part of vmalloc has been more-or-less pure overhead. Allocator
function pointers were maintained here and all calls were indirected through
these function pointers.
This commit removes these function pointers (reducing allocation of the
Vmalloc_t struct by sizeof(void*) * 3) and exposes the (only) implementation
behind these pointers as direct functions that can be called. It is expected
this will accelerate the performance of any lib/expr code heavily using vmalloc.
Apart from performance impact, this is a significant simplification of the
vmalloc code.
This change has no effect on functionality, but strchr is cheaper to call and
equivalent to these strstr calls. This likely makes no difference in an
optimized build as modern compilers can see this transformation is possible
themselves. However, this change may assist older compilers or accelerate
unoptimized builds.
fix: remove hard limit of 1000 boxes in dot spline code
The dot spline code used a static array of 1000 box data structures. It is not
clear to me why this limit was thought to be enough. I suspect the limit (added
in the initial Graphviz revision) was just thought to be something sufficiently
large that no user would ever hit it.
This is no longer true. The graph in issue #2095 exceeds this limit. Because
there are no bounds checks when moving through the boxes array, this resulted in
a crash due to a write beyond the end of the array.
In this commit, we remove this limit entirely and instead use a dynamically
allocated expanding array of boxes. This permits handling an arbitrary number of
boxes during computation. Fixes #2095.
make_flat_edge: use a local box array instead of the global boxes
The values written to this array during make_flat_edge do not need to be
retained after the function returns. It was simply using the boxes as scratch
space. Using a local array instead makes it easier for the compiler to optimize
and will ease some upcoming changes. Related to #2095.
This introduces a new -Wshadow compiler warning, but that will be removed in a
future commit.
make_flat_bottom_edges: use a local box array instead of the global boxes
The values written to this array during make_flat_bottom_edges do not need to be
retained after the function returns. It was simply using the boxes as scratch
space. Using a local array instead makes it easier for the compiler to optimize
and will ease some upcoming changes. Related to #2095.
This introduces a new -Wshadow compiler warning, but that will be removed in a
future commit.
make_flat_labeled_edge: use a local box array instead of the global boxes
The values written to this array during make_flat_labeled_edge do not need to be
retained after the function returns. It was simply using the boxes as scratch
space. Using a local array instead makes it easier for the compiler to optimize
and will ease some upcoming changes. Related to #2095.
This introduces a new -Wshadow compiler warning, but that will be removed in a
future commit.
As I look into how to resolve this issue, I realize it does not make sense to
look for mention of a subgraph because the layout attribute is invalid on *all*
entities except graphs. That is, the error/warning should just say it is only
valid for graphs. Related to #2078.
rewrite streq as a function and remove micro-optimization
There is no need for this to be a macro or for it to check the first character
explicitly. Modern compilers can do this kind of optimization themselves.
I do not know why the comment says it is surprising the assertion fails. It
seems perfectly normal to me that it would fail, as the correct condition is
against *both* AGINEDGE and AGOUTEDGE as in the following line.
There is no need for this to be a macro. Making it a function allows stronger
type safety, safe to use with impure arguments, and reduces bracketing noise.
agrefstrdump: dump dictionary associated with the current graph, not default
It looks like a mistake that this function was ignoring its input argument and
always dumping the default string dictionary. This change makes the function
dump the string dictionary associated with the passed graph. This function is
only compiled in when DEBUG is defined and it is not exposed to users, so no
changelog entry for this. Fixes #1985.
SparseMatrix_copy: avoid calling memcpy with null pointers
The memcpy function technically requires both its pointer inputs to be non-null.
There is no special case for the length being 0. When running the example from
#2088 under UBSan, it detects this memcpy call as being performed with both null
source and destination pointers. It is unlikely any real world memcpy would
misbehave in this circumstance, but it is still good practice to avoid this.
disable -Wunused-parameter when building SWIG-generated Guile bindings
There is no value to warning about things like this in generated code, which are
just artifacts of how SWIG code generation works. Removes 68 compiler warnings.
deal exclusively in size_t in Java bindings support code
In this code, committed in c7374588519a7ac557c14b38c147623dedf43fdb in 2014, it
is not clear to me why int was used. The correct type for these kind of
operations is size_t. The size_t type has been available since at least 1995, if
not earlier depending on your toolchain. Squashes 3
-Wsign-conversion/-Wconversion warnings.
disable -Wunused-variable when building SWIG-generated OCaml bindings
There is no value to warning about things like this in generated code, which are
just artifacts of how SWIG code generation works. Removes 112 compiler warnings.
disable -Wunused-function when building SWIG-generated OCaml bindings
There is no value to warning about things like this in generated code, which are
just artifacts of how SWIG code generation works. Removes 24 compiler warnings.
disable -Wunused-label when building SWIG-generated PHP bindings
There is no value to warning about things like this in generated code, which are
just artifacts of how SWIG code generation works. Removes 95 compiler warnings.
disable -Wunused-parameter when building SWIG-generated PHP bindings
There is no value to warning about things like this in generated code, which are
just artifacts of how SWIG code generation works. Removes 4 compiler warnings.
gv_string_writer: gracefully handle sizes that exceed INT_MAX
Similar to the previous commit, this deals with some mismatch between the TCL
API expecting int and the Graphviz API dealing in size_t.
This stems from a mismatch between gv_channel_write (or back tracking this, the
write_fn member of GVC) dealing in size_t and the TCL API dealing in int. Prior
to this change, a write of more than INT_MAX through this function would result
in passing a negative value to TCL. I do not know how it responds to such
things. Following this change, a write of more than INT_MAX results in a
truncated write of at most INT_MAX, something callers should already be
anticipating. Squashes two compiler warnings.
gv_channel_writer: gracefully handle sizes that exceed INT_MAX
The compiler (correctly) says:
gv_tcl_init.c: In function ‘gv_channel_writer’:
gv_tcl_init.c:25:58: warning: conversion from ‘size_t’ {aka ‘long
unsigned int’} to ‘int’ may change value [-Wconversion]
return Tcl_Write((Tcl_Channel)(job->output_file), s, len);
^~~
gv_tcl_init.c:25:12: warning: conversion to ‘size_t’ {aka ‘long unsigned
int’} from ‘int’ may change the sign of the result [-Wsign-conversion]
return Tcl_Write((Tcl_Channel)(job->output_file), s, len);
^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
This stems from a mismatch between gv_channel_write (or back tracking this, the
write_fn member of GVC) dealing in size_t and the TCL API dealing in int. Prior
to this change, a write of more than INT_MAX through this function would result
in passing a negative value to TCL. I do not know how it responds to such
things. Following this change, a write of more than INT_MAX results in a
truncated write of at most INT_MAX, something callers should already be
anticipating. Squashes two compiler warnings.
disable -Wunused-parameter when building SWIG-generated Ruby bindings
There is no value to warning about things like this in generated code, which are
just artifacts of how SWIG code generation works. Removes 91 compiler warnings.
remove unnecessary not-null guards to free in Operator_diag_precon_delete
It is well-defined to free null. Apart from this though, the first condition
containing a dereference of `o` implies that `o` is not null so the second
condition can be assumed true.
remove unnecessary branch in gvplugin_library_load
The function grealloc can handle the input pointer being NULL and the follow on
code after this does not rely on the contents of the allocated buffer being
zeroed.
projects / graphviz / log