chore: Separate and annotate top-level vs helper functions in rinterface_extra.c#2708
Merged
Conversation
…ra.c `rinterface_extra.c` mixed two functionally distinct kinds of functions with no annotation: - TOP-LEVEL: called from R via `.Call`, registered in the CallEntries[] table in the generated src/cpp11.cpp, and must use IGRAPH_R_CHECK() (which longjmps on error and never returns an error code). - HELPER: called only from other C code; must use IGRAPH_CHECK() and return their igraph_error_t for the caller to propagate. Because the categories were unmarked and the Rx_/Ry_/Rz_/Rw_ name prefixes do not map to them, it was easy to use the wrong error macro. Changes: * Annotate every function definition with a greppable TOP-LEVEL or HELPER marker, and add a file-header doc block defining the two kinds and the IGRAPH_R_CHECK / IGRAPH_CHECK rule. The TOP-LEVEL set is derived from the cpp11.cpp CallEntries registration, not from name prefixes (87 top-level, 171 helper). * Remove 38 dead functions (~1000 lines). These are old hand-written top-level entry points -- Rx_igraph_create, Rx_igraph_star, the whole *_game family (barabasi/watts_strogatz/establishment/...), the st_* connectivity wrappers, Rx_igraph_decompose, Rx_igraph_get_eids, Rx_igraph_graphlets, Rx_igraph_community_leading_eigenvector, the spectral-embedding pair, and a few orphaned helpers (Rx_igraph_get_n, Rx_igraph_get_directed, Rx_igraph_mybracket, ...). Each was long ago superseded by a stimulus-generated R_igraph_* version: they are not registered in cpp11.cpp and not referenced anywhere in src/, R/, tools/ or the yaml/templates, yet they still used IGRAPH_R_CHECK and *looked* top-level -- the single biggest source of the confusion this issue describes. Removal iterated to a fixpoint, also dropping the now-orphaned Rx_igraph_i_levc_callback and its two typedefs. * Fix the one genuine macro inconsistency: Rz_SEXP_to_graph_ptr_list is a helper (returns igraph_error_t, called from generated wrappers in rinterface.c) but used IGRAPH_R_CHECK; switch it to IGRAPH_CHECK. The callers already wrap it in IGRAPH_R_CHECK, so error handling is preserved. The sole remaining helper using IGRAPH_R_CHECK (Rx_igraph_restore_pointer, returns void so it cannot propagate an error code) is documented in its marker. Verified: package compiles and links cleanly; an audit confirms no top-level function uses plain IGRAPH_CHECK and only the one documented void-returning helper uses IGRAPH_R_CHECK; functional spot-checks pass for graph-list operations (union/intersection/decompose, which exercise the modified helper) and for every family whose dead duplicate was removed. Biggest step toward #1056. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rinterface_extra.c
Contributor
Author
|
@krlmlr any todos left here? I think the split into files should be a followup, right? |
Contributor
|
Thanks! Yes, please split as a follow-up. Need to declare the new C file in |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #2105. Biggest step toward #1056.
Background
src/rinterface_extra.cmixes two functionally distinct kinds of functions with no annotation:.Call, registered in theCallEntries[]table in the generatedsrc/cpp11.cpp, and must useIGRAPH_R_CHECK()(which longjmps on error and never returns an error code).IGRAPH_CHECK()and return theirigraph_error_tso the caller can propagate it.Because the categories were unmarked and the
Rx_/Ry_/Rz_/Rw_name prefixes don't map to them, it was easy to use the wrong error macro — exactly the worry raised in #2105.What this PR does
1. Annotate every function + add a header doc block
Each of the 258 function definitions now carries a greppable marker — 87
/* TOP-LEVEL ... */and 171/* HELPER ... */. The TOP-LEVEL set is derived authoritatively from thecpp11.cppCallEntriesregistration table, not from name prefixes. A new file-header block defines the two kinds and theIGRAPH_R_CHECK/IGRAPH_CHECKrule, and notes that the historical prefixes do not indicate the kind.2. Remove 38 dead functions (~1000 lines) — please review carefully
This is the bulk of the diff (1037 deletions). These are old hand-written top-level entry points that were long ago superseded by stimulus-generated
R_igraph_*versions:Rx_igraph_create,Rx_igraph_star,Rx_igraph_ring,Rx_igraph_kary_tree,Rx_igraph_full,Rx_igraph_random_sample,Rx_igraph_get_adjacency,Rx_igraph_get_edge,Rx_igraph_decompose,Rx_igraph_get_eids,Rx_igraph_graphlets,Rx_igraph_community_to_membership,Rx_igraph_community_leading_eigenvector,Rx_igraph_adjacency_spectral_embedding,Rx_igraph_laplacian_spectral_embedding; the whole*_gamefamily (barabasi,barabasi_aging,recent_degree_aging,degree_sequence,callaway_traits,establishment,watts_strogatz,lastcit,cited_type,citing_cited_type); thest_*connectivity / disjoint-paths wrappers (st_vertex_connectivity,st_edge_connectivity,st_mincut_value,edge_disjoint_paths,vertex_disjoint_paths,connect_neighborhood); and a handful of orphaned helpers (Rx_igraph_get_n,Rx_igraph_get_directed,Rx_igraph_mybracket,Rx_igraph_check_finally_stack,Rx_igraph_vectorlist_int_to_SEXP,Rx_igraph_i_levc_arp, plus the cascade-orphanedRx_igraph_i_levc_callbackand its two typedefs).Why they're safe to remove:
cpp11.cpp(CallEntries), so not reachable via.Call.src/,R/,tools/, and the yaml/templates.R_igraph_*replacement exists for each (the R wrappers inR/aaa-*.Rcall the generated versions).They still used
IGRAPH_R_CHECKand looked top-level, which is precisely what made the file confusing. Removal was done programmatically and iterated to a fixpoint so cascade-orphaned helpers were caught too.3. Fix the one genuine macro inconsistency
Rz_SEXP_to_graph_ptr_listis a helper (returnsigraph_error_t, called from generated wrappers inrinterface.c) but usedIGRAPH_R_CHECK. Switched toIGRAPH_CHECK; the callers already wrap it inIGRAPH_R_CHECK, so error semantics are preserved. The only remaining helper that usesIGRAPH_R_CHECKisRx_igraph_restore_pointer, which returnsvoidand therefore cannot propagate an error code — its marker documents why that's intentional.Verification
devtools::load_all).IGRAPH_CHECK, and only the one documentedvoid-returning helper usesIGRAPH_R_CHECK.8173 PASS. The handful of failures are pre-existing flaky/unseeded random-graph tests (test-games.Rbipartite print assertions,cluster_leading_eigen is deterministic) — confirmed to fail/flake identically on the unmodified baseline, so they're unrelated to this change.Rz_SEXP_to_graph_ptr_list) and for every family whose dead duplicate was removed (PA/Barábasi, star, degseq, graphlets, spectral embedding, leading-eigenvector community, max-flow).🤖 Generated with Claude Code