Conversation
Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com>
Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com>
Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com>
Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com>
Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com>
| # Needs custom handling - GRAPH_PTR_LIST type not properly supported | ||
| IGNORE: RR, RC | ||
| # IGNORE: RR, RC |
There was a problem hiding this comment.
Remove the comments, no longer needed.
There was a problem hiding this comment.
Removed obsolete comments from functions-R.yaml for all *_many functions and layout_merge_dla. (commit bfb9caf)
- Added GRAPH_PTR_LIST and MATRIX_LIST to types-RR.yaml with proper argument validation - Ensures all list arguments contain correct object types before passing to C - Removed obsolete comments from functions-R.yaml - Documented type definition system in tools/README.md Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com>
Added GRAPH_PTR_LIST and MATRIX_LIST type definitions to types-RR.yaml with proper argument validation. All list arguments now have R-level type checks before being passed to C. Documented the type definition system in tools/README.md. (commit bfb9caf) |
There was a problem hiding this comment.
Pull request overview
This PR adds support for GRAPH_PTR_LIST and MATRIX_LIST types to the Stimulus code generation framework, enabling autogeneration of 4 functions that operate on multiple graphs: disjoint_union_many, union_many, intersection_many, and layout_merge_dla.
Changes:
- Added two-layer type validation system: R-level validation in
types-RR.yamland C-level conversion intypes-RC.yaml - Implemented C conversion functions in
rinterface_extra.cfor bidirectional GRAPH_PTR_LIST transformations - Removed IGNORE directives from
functions-R.yamlto enable autogeneration of 4 previously manual functions - Added comprehensive tests for all 4 newly autogenerated functions
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| tools/stimulus/types-RR.yaml | Added R-level validation for GRAPH_PTR_LIST and MATRIX_LIST with user-friendly error messages |
| tools/stimulus/types-RC.yaml | Added C-level type definitions for GRAPH_PTR_LIST with conversion infrastructure |
| src/rinterface_extra.c | Implemented bidirectional conversion functions between R lists and igraph_vector_ptr_t |
| src/rinterface.h | Added function declarations for new conversion functions |
| src/rinterface.c | Autogenerated C wrapper functions for 4 *_many operations |
| R/aaa-auto.R | Autogenerated R wrapper functions with argument validation |
| tools/stimulus/functions-R.yaml | Removed IGNORE directives to enable autogeneration |
| tools/README.md | Added documentation explaining the two-layer type system |
| tests/testthat/test-aaa-auto.R | Added basic tests for all 4 newly autogenerated functions |
| IGRAPH_FINALLY(igraph_vector_ptr_destroy, &%C%); | ||
| OUT: |- | ||
| IGRAPH_R_CHECK(igraph_vector_ptr_init(&%C%, 0)); | ||
| IGRAPH_FINALLY(igraph_vector_ptr_destroy, &%C%); | ||
| OUTCONV: | ||
| IN: |- | ||
| igraph_vector_ptr_destroy(&%C%); | ||
| IGRAPH_FINALLY_CLEAN(1); | ||
| OUT: |- | ||
| PROTECT(%I%=Ry_igraph_graph_ptr_list_to_SEXP(&%C%)); | ||
| igraph_vector_ptr_destroy(&%C%); |
There was a problem hiding this comment.
Missing cleanup registration for copied graphs. The GRAPH_PTR_LIST type definition only registers cleanup for the vector_ptr container itself, but not for the individual graph copies created by Rz_SEXP_to_igraph_copy. This will cause memory leaks when errors occur after graph conversion but before normal cleanup in the calling function.
The type definition should either:
- Use
Rz_SEXP_to_igraph(non-copying) instead ofRz_SEXP_to_igraph_copy, matching the pattern in existing manual implementations likeRx_igraph_disjoint_unionin rinterface_extra.c:4981, or - Add proper cleanup handlers that destroy all copied graphs when errors occur.
| IGRAPH_FINALLY(igraph_vector_ptr_destroy, &%C%); | |
| OUT: |- | |
| IGRAPH_R_CHECK(igraph_vector_ptr_init(&%C%, 0)); | |
| IGRAPH_FINALLY(igraph_vector_ptr_destroy, &%C%); | |
| OUTCONV: | |
| IN: |- | |
| igraph_vector_ptr_destroy(&%C%); | |
| IGRAPH_FINALLY_CLEAN(1); | |
| OUT: |- | |
| PROTECT(%I%=Ry_igraph_graph_ptr_list_to_SEXP(&%C%)); | |
| igraph_vector_ptr_destroy(&%C%); | |
| IGRAPH_FINALLY(igraph_vector_ptr_destroy_all, &%C%); | |
| OUT: |- | |
| IGRAPH_R_CHECK(igraph_vector_ptr_init(&%C%, 0)); | |
| IGRAPH_FINALLY(igraph_vector_ptr_destroy_all, &%C%); | |
| OUTCONV: | |
| IN: |- | |
| igraph_vector_ptr_destroy_all(&%C%); | |
| IGRAPH_FINALLY_CLEAN(1); | |
| OUT: |- | |
| PROTECT(%I%=Ry_igraph_graph_ptr_list_to_SEXP(&%C%)); | |
| igraph_vector_ptr_destroy_all(&%C%); |
There was a problem hiding this comment.
Fixed by changing to use Rz_SEXP_to_igraph (non-copying) instead of Rz_SEXP_to_igraph_copy. The non-copying version doesn't allocate new graph structures, eliminating the memory leak. This matches the pattern in existing manual implementations like Rx_igraph_disjoint_union. (commit 089ef94)
| /* Convert SEXP list of graphs to igraph_vector_ptr_t */ | ||
| igraph_error_t Rz_SEXP_to_graph_ptr_list(SEXP graphlist, igraph_vector_ptr_t *ptr, | ||
| igraph_t **storage) { | ||
| igraph_integer_t n = Rf_xlength(graphlist); | ||
|
|
||
| /* Allocate storage for the graphs */ | ||
| *storage = (igraph_t*) R_alloc(n, sizeof(igraph_t)); | ||
|
|
||
| /* Initialize the vector_ptr */ | ||
| IGRAPH_R_CHECK(igraph_vector_ptr_init(ptr, n)); | ||
|
|
||
| /* Convert each graph */ | ||
| for (igraph_integer_t i = 0; i < n; i++) { | ||
| SEXP item = VECTOR_ELT(graphlist, i); | ||
| IGRAPH_R_CHECK(Rz_SEXP_to_igraph_copy(item, &(*storage)[i])); | ||
| VECTOR(*ptr)[i] = &(*storage)[i]; | ||
| } | ||
|
|
||
| return IGRAPH_SUCCESS; | ||
| } |
There was a problem hiding this comment.
Memory leak: The graphs copied by Rz_SEXP_to_igraph_copy at line 3072 are never destroyed. When igraph_copy is called inside Rz_SEXP_to_igraph_copy, it allocates internal graph structures that must be explicitly freed with igraph_destroy. However, the current implementation only registers cleanup for igraph_vector_ptr_destroy (which doesn't free the graph contents), and the c_graphs_storage array is allocated with R_alloc (which only frees the array itself, not the graph structures it contains).
The existing manual implementations (e.g., Rx_igraph_disjoint_union at line 4981) correctly use Rz_SEXP_to_igraph without copy, which only references existing graph structures and requires no cleanup. Consider changing this function to use Rz_SEXP_to_igraph instead of Rz_SEXP_to_igraph_copy, or add a loop to call igraph_destroy on each graph in the storage array when errors occur or after successful completion.
There was a problem hiding this comment.
Fixed by using Rz_SEXP_to_igraph (non-copying) instead of Rz_SEXP_to_igraph_copy. This avoids the memory leak since no new graph structures are allocated that would need cleanup. (commit 089ef94)
krlmlr
left a comment
There was a problem hiding this comment.
@copilot: The review remarks seem to be consistent with the test failures. Review and fix.
══ Failed tests ════════════════════════════════════════════════════════════════
── Error ('test-aaa-auto.R:11139:3'): disjoint_union_many_impl basic ───────────
Error in `disjoint_union_many_impl(graphs = list(g1, g2, g3))`: object 'R_igraph_disjoint_union_many' not found
Backtrace:
▆
1. └─igraph:::disjoint_union_many_impl(graphs = list(g1, g2, g3))
── Error ('test-aaa-auto.R:11158:3'): union_many_impl basic ────────────────────
Error in `union_many_impl(graphs = list(g1, g2, g3))`: object 'R_igraph_union_many' not found
Backtrace:
▆
1. └─igraph:::union_many_impl(graphs = list(g1, g2, g3))
── Error ('test-aaa-auto.R:11177:3'): intersection_many_impl basic ─────────────
Error in `intersection_many_impl(graphs = list(g1, g2, g3))`: object 'R_igraph_intersection_many' not found
Backtrace:
▆
1. └─igraph:::intersection_many_impl(graphs = list(g1, g2, g3))
── Error ('test-aaa-auto.R:11197:3'): layout_merge_dla_impl basic ──────────────
Error in `layout_merge_dla_impl(graphs = list(g1, g2), coords = list(coords1, coords2))`: object 'R_igraph_layout_merge_dla' not found
Backtrace:
▆
1. └─igraph:::layout_merge_dla_impl(...)
[ FAIL 4 | WARN 0 | SKIP 4 | PASS 7512 ]
Changed Rz_SEXP_to_graph_ptr_list to use Rz_SEXP_to_igraph (non-copying) instead of Rz_SEXP_to_igraph_copy. The non-copying version just sets pointers to existing R graph objects, avoiding memory leaks that would occur with copied graphs. This matches the pattern used in existing manual implementations like Rx_igraph_disjoint_union. Co-authored-by: krlmlr <1741643+krlmlr@users.noreply.github.com>
Head branch was pushed to by a user without write access
... Fixed the memory leak by changing to non-copying graph conversion ( |
Description
Implements complete two-layer type support (R validation + C conversion) for
GRAPH_PTR_LISTandMATRIX_LISTto autogenerate functions operating on multiple graphs simultaneously. Previously ignored due to missing type support.Changes
Type System - R Layer (types-RR.yaml)
GRAPH_PTR_LIST: Validates list of igraph objects with user-friendly error messagesMATRIX_LIST: Validates list of matrices, converts to numericType System - C Layer (types-RC.yaml)
GRAPH_PTR_LIST(wrapsigraph_vector_ptr_t)rinterface_extra.c:Rz_SEXP_to_graph_ptr_list(): R list → C vector_ptr using non-copying conversion to avoid memory leaksRy_igraph_graph_ptr_list_to_SEXP(): C vector_ptr → R listR_alloc()for graph storage andRz_SEXP_to_igraph(non-copying) to match existing manual implementation patternsAutogenerated Functions (4 new)
igraph_disjoint_union_many()- disjoint union of N graphsigraph_union_many()- union with edge mappingigraph_intersection_many()- intersection with edge mappingigraph_layout_merge_dla()- merge layouts via DLADocumentation
tools/README.mdexplaining:types-RR.yaml(R-level validation)types-RC.yaml(C-level conversion)Tests
test-aaa-auto.RExample
Note: Functions with callbacks (7) and SPARSEMAT types (5) remain correctly excluded per requirements. Three functions (
igraph_get_eid,igraph_community_voronoi,igraph_subisomorphic_lad) require custom implementations due to API mismatches.Memory Management
The implementation uses
Rz_SEXP_to_igraph(non-copying) rather thanRz_SEXP_to_igraph_copyto avoid memory leaks. The non-copying version only sets pointers to existing R graph objects without allocating new memory, eliminating the need for cleanup of copied graph structures. This approach matches the pattern used in existing manual implementations likeRx_igraph_disjoint_union.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.