Skip to content

Commit 54e5434

Browse files
authored
Fix bug comparing structure types with equivalent() -- false positive (#755)
It was incorrectly saying that two different structs matched! The problem was a subtle issue of the ordering of clauses in a complex comparison -- we need to test the structure case first, because the other clause makes the assumption that the types are simple and not structures and will have a false positive in that case. An important way this could manifest is that if you had two overloaded functions whose arguments were structs, say foo(structA) and foo(structB), it was not able to correctly distinguish between them for type checking.
1 parent 96bb3de commit 54e5434

5 files changed

Lines changed: 58 additions & 12 deletions

File tree

CMakeLists.txt

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -237,6 +237,7 @@ TESTSUITE ( and-or-not-synonyms aastep arithmetic array array-derivs array-range
237237
draw_string
238238
error-dupes exit exponential
239239
function-earlyreturn function-simple function-outputelem
240+
function-overload-struct
240241
geomath getattribute-camera getattribute-shader
241242
getsymbol-nonheap gettextureinfo
242243
group-outputs groupstring

src/liboslexec/typespec.cpp

Lines changed: 15 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -148,19 +148,22 @@ bool
148148
equivalent (const TypeSpec &a, const TypeSpec &b)
149149
{
150150
// The two complex types are equivalent if...
151+
// they are actually identical (duh)
152+
if (a == b)
153+
return true;
154+
// or if they are structs, and the structs are equivalent
155+
if (a.is_structure() || b.is_structure())
156+
return a.is_structure() && b.is_structure() &&
157+
equivalent(a.structspec(), b.structspec());
158+
// or if the underlying simple types are equivalent
151159
return
152-
// they are actually identical (duh)
153-
(a == b) ||
154-
// or if the underlying simple types are equivalent
155-
(((a.is_vectriple_based() && b.is_vectriple_based()) || equivalent(a.m_simple, b.m_simple)) &&
156-
// ... and either both or neither are closures
157-
a.is_closure() == b.is_closure() &&
158-
// ... and, if arrays, they are the same length, or both unsized,
159-
// or one is unsized and the other isn't
160-
(a.m_simple.arraylen == b.m_simple.arraylen || a.is_unsized_array() != b.is_unsized_array())) ||
161-
// or if they are structs, and the structs are equivalent
162-
(a.is_structure() && b.is_structure() &&
163-
equivalent(a.structspec(), b.structspec()));
160+
((a.is_vectriple_based() && b.is_vectriple_based()) || equivalent(a.m_simple, b.m_simple))
161+
// ... and either both or neither are closures
162+
&& a.is_closure() == b.is_closure()
163+
// ... and, if arrays, they are the same length, or both unsized,
164+
// or one is unsized and the other isn't
165+
&& (a.m_simple.arraylen == b.m_simple.arraylen ||
166+
a.is_unsized_array() != b.is_unsized_array());
164167
}
165168

166169

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
Compiled test.osl -> test.oso
2+
c2_out = 0.3 0.3
3+
Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,3 @@
1+
#!/usr/bin/env python
2+
3+
command = testshade("test")
Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,36 @@
1+
struct color2
2+
{
3+
float r;
4+
float g;
5+
};
6+
7+
struct color4
8+
{
9+
color rgb;
10+
float a;
11+
};
12+
13+
14+
color2 __operator__add__(color2 c1, color2 c2)
15+
{
16+
return color2(c1.r + c2.r, c1.g + c2.g);
17+
}
18+
19+
color4 __operator__add__(color4 c1, color4 c2)
20+
{
21+
return color4(c1.rgb + c2.rgb, c1.a + c2.a);
22+
}
23+
24+
25+
shader test
26+
(
27+
color2 c2_in_1 = {.1, .1},
28+
color2 c2_in_2 = {.2, .2},
29+
30+
output color2 c2_out = {0, 0},
31+
)
32+
{
33+
c2_out = c2_in_1 + c2_in_2;
34+
printf ("c2_out = %g %g\n", c2_out.r, c2_out.g);
35+
}
36+

0 commit comments

Comments
 (0)