chore: R_SEXP_to_igraph() and R_SEXP_to_igraph_copy() get igraph_t object from external pointer

[Antonov548]

No description provided.

[Antonov548]

refactoring for 0.10 to handle cache in igraph_t

[krlmlr]

Brilliant, thanks! I could release this and more refactorings (if needed) as 1.5.1.

Can you please merge this into #840 first?

[Antonov548]

oldrel-1

Yes, I will merge it. oldrel-1 failed for some reason which is not related to changes I think.

[krlmlr]

This does look like a protection error, but it's unclear where that came from.

Checking on top of v1.5.0: https://github.com/igraph/rigraph/actions/runs/5437288024

[krlmlr]

Also on the new r-1.5 branch, with R 4.2 on my system:

options(conflicts.policy = list(warn = FALSE))
library(igraph)

g <- make_ring(3)

gctorture()
for (i in 1:3) {
  print(i)
  g <- add_vertices(g, 3)
}
#> [1] 1
#> Error in add_vertices(g, 3): VECTOR_ELT() can only be applied to a 'list', not a 'integer'

^{Created on 2023-07-02 with reprex v2.0.2}

Can you replicate?

[krlmlr]

Something's not quite right in the realm of attribute handling. I added a commit that fixes the problem, but I don't think this should be even necessary. That commit might introduce a memory leak: R objects that are never freed.

I don't understand the global (!!!) R_igraph_attribute_protected and R_igraph_attribute_protected_size variables. I do think we need to step back and, once more, clean this up before proceeding with 0.10.

[krlmlr]

This is now mergeable, but we should definitely understand what's going on here.

[krlmlr]

I started revdepchecks on the current main branch. If they pass, we can continue working on the main branch, and switch to "baby step" mode again.

[krlmlr]

@Antonov548: I'm pretty sure that my last commit is bad, but I can't quite understand how to fix correctly.

[Antonov548]

I'm pretty sure that my last commit is bad, but I can't quite understand how to fix correctly.

I also not sure about this changes. I tried play around with attributes - this changes works but still don't understand in details how they works. I think if this preserve also should be in the copy graph function.

[szhorvat]

I am not familiar with R's C API, I just wanted to point out that igraph_copy() will invoke R_igraph_attribute_copy(). If that function does the attribute copying properly, then R_SEXP_to_igraph_copy() should not change attributes explicitly.

[krlmlr]

This runs correctly on top of #870.

[krlmlr]

It's weird that I can't seem to add a test in the spirit of https://github.com/igraph/rigraph/pull/870/files#diff-2f53a23080d87ca119f5c65194b7cf9df808dd6e77ac201309b45f4dd7231830R39-R47 . But we can still check manually before merging.

Let's merge the other PRs and further simplify this PR afterward.

[krlmlr]

I updated with rebase, this amounts to a force-push.

Can you please further simplify? I think both routines shouldn't need to touch the ->attr component anymore.

[Antonov548]

Can you please further simplify? I think both routines shouldn't need to touch the ->attr component anymore.

84 tests are failed if ->attr is not assigned.

[Antonov548]

Can you please further simplify? I think both routines shouldn't need to touch the ->attr component anymore.

I can't get why it's not needed anymore. Since attributes is still part of list structure with igraph_t_idx_attr index.

[krlmlr]

I think I understand why we need the attribute copy in R_SEXP_to_igraph() : The attributes may have changed from the R side without updating the C structure, this takes care of it.

I don't understand why we need it in R_SEXP_to_igraph_copy(), I added a FIXME so that we can review later.

[Antonov548]

@krlmlr are we okay to merge this?

[krlmlr]

Thanks!