Closed JamesWrigley closed 1 year ago
Thanks for fixing this. Did it solve #440 as well?
No unfortunately, I'm still looking into that. My current suspicion is that there's something going wrong with the cycle detection :thinking: Just for my own information, am I correct in thinking that StructMutualRef
and the RemoveCircularReference
pass is only for removing cycles like these?
typedef struct Bar;
struct Foo {
Bar* bar;
};
struct Bar {
Foo foo; // Could also be a pointer
}
Yes. I'm now trying to reduce the header to an MWE.
MWE:
typedef struct A A;
typedef struct B B;
typedef struct C C;
struct B;
struct C;
struct B
{
B* x;
};
struct C
{
B y[10];
};
struct A
{
C* z;
};
With this PR:
julia> using Clang.Generators
[ Info: Precompiling Clang [40e3b903-d033-50b4-a0cc-940c62c95e31]
julia> args = get_default_args()
6-element Vector{String}:
"-isystem/Users/gnimuc/.julia/ar" ⋯ 61 bytes ⋯ "64-apple-darwin14/4.8.5/include"
"-isystem/Users/gnimuc/.julia/ar" ⋯ 67 bytes ⋯ "le-darwin14/4.8.5/include-fixed"
"-isystem/Users/gnimuc/.julia/ar" ⋯ 47 bytes ⋯ "9/x86_64-apple-darwin14/include"
"-isystem/Users/gnimuc/.julia/ar" ⋯ 60 bytes ⋯ "e-darwin14/sys-root/usr/include"
"-isystem/Users/gnimuc/.julia/ar" ⋯ 74 bytes ⋯ "-root/System/Library/Frameworks"
"--target=x86_64-apple-darwin14"
julia> ctx = create_context("test.h", args)
[ Info: Parsing headers...
Context(...)
julia> build!(ctx)
[ Info: Processing header: test.h
[ Info: Building the DAG...
┌ Warning: default libname: ":libxxx" is being used, did you forget to set `library_name` in the toml file?
└ @ Clang.Generators ~/.julia/dev/Clang/src/generator/audit.jl:16
[ Info: Emit Julia expressions...
struct A
z::Ptr{Cvoid} # z::Ptr{C}
end
function Base.getproperty(x::A, f::Symbol)
f === :z && return Ptr{C}(getfield(x, f))
return getfield(x, f)
end
struct B
x::Ptr{B}
end
struct C
y::NTuple{10, B}
end
[ Info: Done!
Context(...)
The simplified MWE works, but it's not equivalent to your original MWE which still fails for me. I think this is an equivalent MWE:
typedef struct A A;
typedef struct B B;
typedef struct C C;
struct B;
struct C;
struct B
{
B* x;
};
struct C
{
B y[10];
};
typedef struct vector_C
{
C* c;
} vector_C;
typedef struct pool_C
{
vector_C vc;
} pool_C;
struct A
{
pool_C pc;
C* c;
};
Ok, I stepped through detect_cycle!()
and couldn't find any issues, so now I'm thinking that the cycle removal is too aggressive :thinking: Currently it's looping through each each parent/child in the whole cycle path and (after checking some conditions) converts each to a StructMutualRef
. But I think it's only necessary to convert the leaf parent/child since that's the actual cycle?
That would explain the output:
[ Info: Building the DAG...
[ Info: [RemoveCircularReference]: removed ImPlotAxis's dependency ImPlotAxis
[ Info: [RemoveCircularReference]: removed ImPlotPlot's dependency ImPlotAxis
[ Info: [RemoveCircularReference]: removed ImVector_ImPlotPlot's dependency ImPlotPlot
It's considering ImPlotAxis
a mutual reference with itself, but then it's continuing up the cycle and converting the parents ImPlotPlot
and ImVector_ImPlotPlot
too.
62c9917 fixes the issue for me :sweat_smile:
Ok, I think this is ready for review. I improved the original error message in 78bcaf8 and added a test for the cycle detection in 761b9e7.
(BTW if you approve it I can squash the fixup commits before you merge)
The
i
index in the outermost loop over the nodes would become shadowed by thei
index in the inner loop over the cycle indices, so the check to exit early would be done against the wrong variable.I don't have an example of this failing, I was just reading the code and figured that it was probably a bug. Unfortunately I'm not familiar enough with the code to write a test for it.