Skip to content

fix: initialize c_options in the Stimulus type definition - #2792

Merged
krlmlr merged 2 commits into
mainfrom
claude/stimulus-2751-adaptation-e97p3k
Jul 27, 2026
Merged

fix: initialize c_options in the Stimulus type definition#2792
krlmlr merged 2 commits into
mainfrom
claude/stimulus-2751-adaptation-e97p3k

Conversation

@krlmlr

@krlmlr krlmlr commented Jul 27, 2026

Copy link
Copy Markdown
Contributor

Stacked on #2751: the first commit is @MichaelChirico's fix to src/rinterface.c, the second moves it into the code generator so that it survives the next regeneration.
GitHub cannot use a fork branch as a PR base, so the commit is carried along here instead of being a true base branch; it can be dropped once #2751 is merged.

The fix

PAGERANKOPT is an OPTIONAL INOUT argument, so Stimulus wraps its input conversion into if (!Rf_isNull(options)), while chunk_call() adds the (Rf_isNull(...) ? 0 : ...) guard only for arguments that are purely input.
The pointer is therefore read unconditionally at the call site, which is an uninitialized read when options is NULL at the R level, as flagged by clang -Wsometimes-uninitialized.

The type definition now declares the variable through CDECL with a NULL initializer:

PAGERANKOPT:
    CTYPE: void*
    CDECL: |-
      igraph_arpack_options_t %C%1;
      void* %C% = NULL;

CTYPE no longer has to smuggle the extra igraph_arpack_options_t declaration past the type, so it can name the actual C type again.

The else { %C% = NULL; } branch of the input conversion is dropped: with the initializer in place it is a redundant re-assignment, and keeping it would suggest that the two NULL paths differ.
The default now lives in exactly one place.

Generated code for the three affected functions (igraph_pagerank(), igraph_personalized_pagerank(), igraph_personalized_pagerank_vs()):

   igraph_arpack_options_t c_options1;
-  void* c_options;
+  void* c_options = NULL;
...
   if (!Rf_isNull(options)) {
     if (c_algo == IGRAPH_PAGERANK_ALGO_ARPACK) {
       Rz_SEXP_to_igraph_arpack_options(options, &c_options1);
       c_options = &c_options1;
-    } else {
-      c_options = NULL;
     }
   }

Are other functions affected?

No, these three are the only real cases.
Compiling the regenerated src/rinterface.c with clang -Wall -Wextra reports no -Wsometimes-uninitialized warning any more.

Five further sites are reported by gcc -O2 -Wmaybe-uninitialized and clang -Wconditional-uninitialized: c_outfile in igraph_maximal_cliques_subset(), c_creator in igraph_write_graph_gml(), c_start in igraph_fundamental_cycles() and igraph_vertex_path_from_edge_path(), and c_vid in igraph_random_spanning_tree().
These are false positives: the arguments are purely input, so the call site does carry the (Rf_isNull(x) ? 0 : c_x) guard and never reads the variable on the path where the conversion is skipped.
The compilers just do not correlate the two Rf_isNull() tests.
Silencing them would mean initializing every optional scalar declaration, which would churn the generated code and weaken the warning elsewhere, so this PR leaves them alone.

  • By submitting this pull request, I assign the copyright of my contribution to The igraph development team.

🤖 Generated with Claude Code

https://claude.ai/code/session_01RF5eavxtBTfTSquRjc2San


Generated by Claude Code

MichaelChirico and others added 2 commits July 27, 2026 04:31
The `PAGERANKOPT` type declares an optional `INOUT` argument,
so Stimulus wraps its input conversion into `if (!Rf_isNull(options))`,
but passes the pointer to `igraph_pagerank()` unconditionally.
Reading it when `options` is `NULL` at the R level is an uninitialized read,
flagged by `clang -Wsometimes-uninitialized`.

Declare the pointer with a `NULL` initializer via `CDECL`,
which is the same fix as in #2751 but applied to the code generator.
The `else` branch of the input conversion becomes redundant and is dropped,
so the default lives in one place only.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RF5eavxtBTfTSquRjc2San
@MichaelChirico

Copy link
Copy Markdown
Contributor

GitHub cannot use a fork branch as a PR base, so the commit is carried along here instead of being a true base branch

I'm curious what Claude is doing internally to manage a stacked chain, is it anything formal? or it's just keeping track of it "manually"?

E.g. keeping a clone of my forked branch to use as a base here would be natural but Claude doesn't.

(fwiw I've become quite fond of jj lately)

@krlmlr

krlmlr commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

I just requested a stacked PR and it figured it out. Using Claude Code Web which has some restrictions.

Do you mean https://github.com/jj-vcs/jj? This does look interesting. What do you like about it? Are we losing any features under this abstraction, or can we always drop to the underlying layer?

@krlmlr
krlmlr merged commit 2f26d31 into main Jul 27, 2026
6 checks passed
@krlmlr
krlmlr deleted the claude/stimulus-2751-adaptation-e97p3k branch July 27, 2026 05:51
@MichaelChirico

Copy link
Copy Markdown
Contributor

What do you like about it?

The language for moving commits around on a tree feels pretty natural to me, as does the syntax for describing revsets (@ for "here", @- for "parent of here", xx- for "parent of 'xx'", xx::yy for "'xx' through 'yy'"). And being able to jj undo (or jj op restore) to back yourself out of corners is very helpful.

I also use more than one VCS day-to-day and being able to use the more-or-less the same syntax is quite a relief.

Are we losing any features under this abstraction, or can we always drop to the underlying layer?

For my own workflows, I'm still adapting, but I basically use git like (1) plain old git for simple branches, one bug fix, a simple feature, etc; or (2) fully jj for a complicated series, without any direct git invocations.

The interaction with GitHub is still a bit awkward, but it's a bit a limitation of git itself. There are tons of --force pushes to remap the git history to match the jj graph, which you don't notice from the CLI but they pollute the PR page on GitHub. Also, GitHub's handling of chains is still less-than-ideal (manually setting the PR target and updating it as things get merged/moved around).

I just requested a stacked PR

Is that the new GitHub stacked PRs feature (link)? I haven't gotten a chance to try that out yet.

@krlmlr

krlmlr commented Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

I just signed up for the stacked PRs preview.

Claude also force-pushes a lot under my guidance. I use Git pretty much exclusively and am not ready to embrace a new mental model. For newcomers, this looks like an interesting approach to getting accustomed to VCS.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants