fix GetVariableOffset in cases of multi-level and multiple inheritance
#396
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: parameter 'b' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:270: expanded from macro 'CODE'
BaseB(int x, std::string b) : BaseA(x), b(b) {} \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: parameter 's' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:277: expanded from macro 'CODE'
Base1(int i, std::string s) : i(i), s(s) {} \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: pass by value and use std::move [modernize-pass-by-value]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:270: expanded from macro 'CODE'
BaseB(int x, std::string b) : BaseA(x), b(b) {} \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: variable 'my_k' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:285: expanded from macro 'CODE'
} my_k(5, 4, 3, "Cpp", "Python");
^| TEST(VariableReflectionTest, VariableOffsetsWithInheritance) { | ||
| Cpp::Declare("#include<string>"); | ||
|
|
||
| #define Stringify(s) Stringifyx(s) |
There was a problem hiding this comment.
warning: function-like macro 'Stringify' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
#define Stringify(s) Stringifyx(s)
^| Cpp::Declare("#include<string>"); | ||
|
|
||
| #define Stringify(s) Stringifyx(s) | ||
| #define Stringifyx(...) #__VA_ARGS__ |
There was a problem hiding this comment.
warning: variadic macro 'Stringifyx' used; consider using a 'constexpr' variadic template function [cppcoreguidelines-macro-usage]
#define Stringifyx(...) #__VA_ARGS__
^| Cpp::TCppScope_t myklass = Cpp::GetNamed("MyKlass"); | ||
| EXPECT_TRUE(myklass); | ||
|
|
||
| size_t num_bases = Cpp::GetNumBases(myklass); |
There was a problem hiding this comment.
warning: no header providing "size_t" is directly included [misc-include-cleaner]
unittests/CppInterOp/VariableReflectionTest.cpp:6:
+ #include <cstddef>| EXPECT_EQ(datamembers.size(), 5); | ||
|
|
||
| EXPECT_EQ(Cpp::GetVariableOffset(datamembers[0], myklass), | ||
| ((intptr_t)&(my_k.k)) - ((intptr_t)&(my_k))); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
((intptr_t)&(my_k.k)) - ((intptr_t)&(my_k)));
^
|
We should probably turn off clang tidy from the unittests. |
|
Looks like, if we include a virtual destructor in the base classes these changes are not required. EDIT class BaseA {};
class BaseB: public BaseA {};But does not work with multiple inheritance i.e. class BaseA {};
class BaseB {};
class Klass: public BaseA, public BaseB {}; |
Vipul-Cariappa
force-pushed
the
dev/offset-inheritance
branch
from
622d14f to
e2082f3
Compare
| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: class 'Base1' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:275: expanded from macro 'CODE'
class Base1 { \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: class 'BaseA' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:261: expanded from macro 'CODE'
class BaseA { \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: class 'BaseB' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:268: expanded from macro 'CODE'
class BaseB : public BaseA { \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: class 'MyKlass' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:283: expanded from macro 'CODE'
class MyKlass : public BaseB, public Base1 { \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: parameter 'b' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:272: expanded from macro 'CODE'
BaseB(int x, std::string b) : BaseA(x), b(b) {} \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: parameter 's' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:280: expanded from macro 'CODE'
Base1(int i, std::string s) : i(i), s(s) {} \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: pass by value and use std::move [modernize-pass-by-value]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:272: expanded from macro 'CODE'
BaseB(int x, std::string b) : BaseA(x), b(b) {} \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: variable 'my_k' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:289: expanded from macro 'CODE'
} my_k(5, 4, 3, "Cpp", "Python");
^| EXPECT_EQ(datamembers.size(), 5); | ||
|
|
||
| EXPECT_EQ(Cpp::GetVariableOffset(datamembers[0], myklass), | ||
| ((intptr_t)&(my_k.k)) - ((intptr_t)&(my_k))); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
((intptr_t)&(my_k.k)) - ((intptr_t)&(my_k)));
^| ((intptr_t)&(my_k.k)) - ((intptr_t)&(my_k))); | ||
|
|
||
| EXPECT_EQ(Cpp::GetVariableOffset(datamembers[1], myklass), | ||
| ((intptr_t)&(my_k.a)) - ((intptr_t)&(my_k))); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
((intptr_t)&(my_k.a)) - ((intptr_t)&(my_k)));
^
Vipul-Cariappa
force-pushed
the
dev/offset-inheritance
branch
from
e2082f3 to
16f72ad
Compare
| ((intptr_t)&(my_k.k)) - ((intptr_t)&(my_k))); | ||
|
|
||
| EXPECT_EQ(Cpp::GetVariableOffset(datamembers[1], myklass), | ||
| ((intptr_t)&(my_k.a)) - ((intptr_t)&(my_k))); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
((intptr_t)&(my_k.a)) - ((intptr_t)&(my_k)));
^| ((intptr_t)&(my_k.a)) - ((intptr_t)&(my_k))); | ||
|
|
||
| EXPECT_EQ(Cpp::GetVariableOffset(datamembers[2], myklass), | ||
| ((intptr_t)&(my_k.b)) - ((intptr_t)&(my_k))); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
((intptr_t)&(my_k.b)) - ((intptr_t)&(my_k)));
^| ((intptr_t)&(my_k.a)) - ((intptr_t)&(my_k))); | ||
|
|
||
| EXPECT_EQ(Cpp::GetVariableOffset(datamembers[2], myklass), | ||
| ((intptr_t)&(my_k.b)) - ((intptr_t)&(my_k))); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
((intptr_t)&(my_k.b)) - ((intptr_t)&(my_k)));
^| ((intptr_t)&(my_k.b)) - ((intptr_t)&(my_k))); | ||
|
|
||
| EXPECT_EQ(Cpp::GetVariableOffset(datamembers[3], myklass), | ||
| ((intptr_t)&(my_k.i)) - ((intptr_t)&(my_k))); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
((intptr_t)&(my_k.i)) - ((intptr_t)&(my_k)));
^| ((intptr_t)&(my_k.b)) - ((intptr_t)&(my_k))); | ||
|
|
||
| EXPECT_EQ(Cpp::GetVariableOffset(datamembers[3], myklass), | ||
| ((intptr_t)&(my_k.i)) - ((intptr_t)&(my_k))); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
((intptr_t)&(my_k.i)) - ((intptr_t)&(my_k)));
^| ((intptr_t)&(my_k.i)) - ((intptr_t)&(my_k))); | ||
|
|
||
| EXPECT_EQ(Cpp::GetVariableOffset(datamembers[4], myklass), | ||
| ((intptr_t)&(my_k.s)) - ((intptr_t)&(my_k))); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
((intptr_t)&(my_k.s)) - ((intptr_t)&(my_k)));
^| ((intptr_t)&(my_k.i)) - ((intptr_t)&(my_k))); | ||
|
|
||
| EXPECT_EQ(Cpp::GetVariableOffset(datamembers[4], myklass), | ||
| ((intptr_t)&(my_k.s)) - ((intptr_t)&(my_k))); |
There was a problem hiding this comment.
warning: do not use C-style cast to convert between unrelated types [cppcoreguidelines-pro-type-cstyle-cast]
((intptr_t)&(my_k.s)) - ((intptr_t)&(my_k)));
^
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #396 +/- ##
==========================================
+ Coverage 70.62% 70.84% +0.21%
==========================================
Files 9 9
Lines 3500 3529 +29
==========================================
+ Hits 2472 2500 +28
- Misses 1028 1029 +1
|
@vgvassilev @Vipul-Cariappa If you want clang tidy to not provide comments on unit tests then you'll want to change CppInterOp/.github/workflows/clang-format.yml Lines 6 to 7 in c71069f |
Vipul-Cariappa
force-pushed
the
dev/offset-inheritance
branch
from
16f72ad to
0e30059
Compare
| @@ -7,6 +7,8 @@ | |||
|
|
|||
| #include "gtest/gtest.h" | |||
There was a problem hiding this comment.
warning: 'gtest/gtest.h' file not found [clang-diagnostic-error]
#include "gtest/gtest.h"
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: class 'Base1' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:277: expanded from macro 'CODE'
class Base1 { \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: class 'BaseA' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:263: expanded from macro 'CODE'
class BaseA { \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: class 'BaseB' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:270: expanded from macro 'CODE'
class BaseB : public BaseA { \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: class 'MyKlass' defines a non-default destructor but does not define a copy constructor, a copy assignment operator, a move constructor or a move assignment operator [cppcoreguidelines-special-member-functions]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:285: expanded from macro 'CODE'
class MyKlass : public BaseB, public Base1 { \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: parameter 'b' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:274: expanded from macro 'CODE'
BaseB(int x, std::string b) : BaseA(x), b(b) {} \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: parameter 's' is passed by value and only copied once; consider moving it to avoid unnecessary copies [performance-unnecessary-value-param]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:282: expanded from macro 'CODE'
Base1(int i, std::string s) : i(i), s(s) {} \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: pass by value and use std::move [modernize-pass-by-value]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:274: expanded from macro 'CODE'
BaseB(int x, std::string b) : BaseA(x), b(b) {} \
^| : BaseB(x, b), Base1(i, s), k(k) {} \ | ||
| } my_k(5, 4, 3, "Cpp", "Python"); | ||
|
|
||
| CODE |
There was a problem hiding this comment.
warning: variable 'my_k' is non-const and globally accessible, consider making it const [cppcoreguidelines-avoid-non-const-global-variables]
CODE
^Additional context
unittests/CppInterOp/VariableReflectionTest.cpp:291: expanded from macro 'CODE'
} my_k(5, 4, 3, "Cpp", "Python");
^
Description
Fixes the calculation of data member offsets of a class C. Where C inherits from multiple classes and these classes themselves inherit from other classes.
Fixes # (issue)
I found this issue when tinkering with cross-inheritance in cppyy.
It looks like upstream cppyy also faces a similar problem. Opened a PR stating the issue at wlav/cppyy#280.
Type of change
Please tick all options which are relevant.
Testing
Added in tests with multi-level and multiple inheritance.
Checklist