Skip to content

Fixes for cv-qualified arrays #35

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 1 commit into from
Apr 26, 2016
Merged

Fixes for cv-qualified arrays #35

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
61 changes: 52 additions & 9 deletions pygccxml/declarations/type_traits.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,19 +284,46 @@ def remove_reference(type):
def is_const(type):
"""returns True, if type represents C++ const type, False otherwise"""
nake_type = remove_alias(type)
return isinstance(nake_type, cpptypes.const_t)
if isinstance(nake_type, cpptypes.const_t):
return True
elif isinstance(nake_type, cpptypes.volatile_t):
return is_const(nake_type.base)
elif isinstance(nake_type, cpptypes.array_t):
return is_const(nake_type.base)
return False


def remove_const(type):
def remove_const(type_):
"""removes const from the type definition

If type is not const type, it will be returned as is
"""

nake_type = remove_alias(type)
nake_type = remove_alias(type_)
if not is_const(nake_type):
return type
return type_
else:
# Handling for const and volatile qualified types. There is a
# difference in behavior between GCCXML and CastXML for cv-qual arrays.
# GCCXML produces the following nesting of types:
# -> volatile_t(const_t(array_t))
# while CastXML produces the following nesting:
# -> array_t(volatile_t(const_t))
# For both cases, we must unwrap the types, remove const_t, and add
# back the outer layers
if isinstance(nake_type, cpptypes.array_t):
is_v = is_volatile(nake_type)
if is_v:
result_type = nake_type.base.base.base
else:
result_type = nake_type.base.base
if is_v:
result_type = cpptypes.volatile_t(result_type)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here you are generating new instances, and return them. I think (I did not test this) that this may be a problem if a comparison needs to be done (using is_same() method for example). Could the types be retrieved by returning the right .base ?
The same comment holds for the remove_volatile() method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understand. If you have a type, say t, which is const int, then is_same(t, remove_const(t)) will return False, which is correct. But is_same(remove_const(t), remove_const(t)) should still return True (but I haven't tested this).

I don't think you can get these right without creating new instances; what you have is nesting like this - array_t -> const_t -> (type), and you need to remove the const_t from that hierarchy. But if you can think of a cleaner way to do this, I'm all for it. I really dislike the nake_type.base.base.base part.

return cpptypes.array_t(result_type, nake_type.size)

elif isinstance(nake_type, cpptypes.volatile_t):
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

remove_const wasn't fully fixed by the previous commits. The code I added above took care of T const volatile[N] types, but simple T const volatile types were still not being handled correctly. This elif takes care of that case.

return cpptypes.volatile_t(nake_type.base.base)

return nake_type.base


Expand All @@ -322,7 +349,13 @@ def is_same(type1, type2):
def is_volatile(type):
"""returns True, if type represents C++ volatile type, False otherwise"""
nake_type = remove_alias(type)
return isinstance(nake_type, cpptypes.volatile_t)
if isinstance(nake_type, cpptypes.volatile_t):
return True
elif isinstance(nake_type, cpptypes.const_t):
return is_volatile(nake_type.base)
elif isinstance(nake_type, cpptypes.array_t):
return is_volatile(nake_type.base)
return False


def remove_volatile(type):
Expand All @@ -334,6 +367,16 @@ def remove_volatile(type):
if not is_volatile(nake_type):
return type
else:
if isinstance(nake_type, cpptypes.array_t):
is_c = is_const(nake_type)
if is_c:
base_type = nake_type.base.base.base
else:
base_type = nake_type.base.base
result_type = base_type
if is_c:
result_type = cpptypes.const_t(result_type)
return cpptypes.array_t(result_type, nake_type.size)
return nake_type.base


Expand All @@ -344,12 +387,12 @@ def remove_cv(type):
if not is_const(nake_type) and not is_volatile(nake_type):
return type
result = nake_type
if is_const(nake_type):
result = nake_type.base
if is_const(result):
result = remove_const(result)
if is_volatile(result):
result = result.base
result = remove_volatile(result)
if is_const(result):
result = result.base
result = remove_const(result)
return result


Expand Down
59 changes: 59 additions & 0 deletions unittests/array_bug_tester.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,65 @@ def test4(self):
'::xyz[2][3]' == aaaa_type.decl_string,
aaaa_type.decl_string)

def test5(self):
code = 'char const arr[4] = {};'
src_reader = parser.source_reader_t(self.config)
global_ns = declarations.get_global_namespace(
src_reader.read_string(code))
arr_type = global_ns.variable('arr').decl_type
if self.config.xml_generator == "gccxml":
self.assertTrue(
'char[4] const' == arr_type.decl_string,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to you that gccxml is putting the cv-qualifiers on the right of the array declarator in decl_string?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No it doesn't make sense. I guess this is a gccxml bug. From what I know, the gccxml project did a lot of work to produce the xml file, and the output was not always very "clean". This may also be due to how it interacted with gcc.

With castxml, we rely on the AST produced by clang/llvm. And this AST is much more robust, as far I have seen. The ordering in the xml file is always correct (there were other cases were the output was nicer when I added support for CastXML).

So just leave the tests like this. It just makes sure the results stay the same in the future. CastXML will be the default xml parser in release 1.8.0, and gccxml support will be dropped in further releases, so these gccxml specific checks will be dropped anyway.

arr_type.decl_string)
else:
self.assertTrue(
'char const[4]' == arr_type.decl_string,
arr_type.decl_string)
self.assertTrue(
declarations.is_array(arr_type))
self.assertTrue(
declarations.is_const(arr_type))

def test6(self):
code = 'char volatile arr[4] = {};'
src_reader = parser.source_reader_t(self.config)
global_ns = declarations.get_global_namespace(
src_reader.read_string(code))
arr_type = global_ns.variable('arr').decl_type
if self.config.xml_generator == "gccxml":
self.assertTrue(
'char[4] volatile' == arr_type.decl_string,
arr_type.decl_string)
else:
self.assertTrue(
'char volatile[4]' == arr_type.decl_string,
arr_type.decl_string)
self.assertTrue(
declarations.is_array(arr_type))
self.assertTrue(
declarations.is_volatile(arr_type))

def test7(self):
code = 'char const volatile arr[4] = {};'
src_reader = parser.source_reader_t(self.config)
global_ns = declarations.get_global_namespace(
src_reader.read_string(code))
arr_type = global_ns.variable('arr').decl_type
if self.config.xml_generator == "gccxml":
self.assertTrue(
'char[4] const volatile' == arr_type.decl_string,
arr_type.decl_string)
else:
self.assertTrue(
'char const volatile[4]' == arr_type.decl_string,
arr_type.decl_string)
self.assertTrue(
declarations.is_array(arr_type))
self.assertTrue(
declarations.is_const(arr_type))
self.assertTrue(
declarations.is_volatile(arr_type))


def create_suite():
suite = unittest.TestSuite()
Expand Down
74 changes: 60 additions & 14 deletions unittests/data/type_traits_gccxml.hpp → unittests/data/type_traits.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@
typedef BASE volatile NAME##_volatile_t; \
typedef BASE const volatile NAME##_const_volatile_t;


struct some_struct_t{
void do_smth();
int member;
Expand Down Expand Up @@ -47,9 +48,7 @@ struct incomplete_type;

namespace is_void{
namespace yes{
typedef void void_t;
typedef void const void_cont_t;
typedef void volatile void_volatile_t;
TYPE_PERMUTATION( void, void )
}
namespace no{
typedef void* void_ptr_t;
Expand Down Expand Up @@ -186,7 +185,6 @@ namespace yes{
typedef detail::dd_t dd_t;
typedef detail::f_t f_t;
typedef detail::g_t g_t;

typedef detail::const_container const_container_t;
typedef detail::const_item const_item_t;

Expand Down Expand Up @@ -287,11 +285,6 @@ namespace no{
namespace is_fundamental{
namespace yes{

#define FUNDAMENTAL_TYPE_PERMUTATION( BASE, NAME ) \
typedef BASE NAME##_t; \
typedef BASE const NAME##_const_t; \
typedef BASE volatile NAME##_volatile_t;

TYPE_PERMUTATION( void, void )
TYPE_PERMUTATION( bool, bool )
TYPE_PERMUTATION( char, char )
Expand Down Expand Up @@ -409,7 +402,8 @@ namespace yes{
typedef const void const_void_t;
typedef const incomplete_type const_incomplete_type_t;
typedef int* const int_const_t;
//TODO typedef const int& const_int_ref_t;
typedef int* volatile const int_volatile_const_t;
typedef int* const volatile int_const_volatile_t;
}

namespace no{
Expand All @@ -421,32 +415,53 @@ namespace no{
typedef void(*function_t)();
typedef void (some_struct_t::*member_function_t)();
typedef int int_t;
typedef const int& const_int_ref_t;
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved this from yes to no. is_const should return False for int const&, see this question I answered on SO a while back.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. I already stumbled on this test case, and had the same reflection, but did not dig further.

Looking at the git history, the typedef const int& const_int_ref_t; test was added a little bit later than the other tests; and the is_const part was directly commented out so there was already a doubt on what to expect here.

+1 on that change

} }

namespace remove_const{
namespace before{

typedef const void x1;
typedef const incomplete_type x2;
typedef int* const x3;
typedef int* volatile x4;
typedef void const * x5;
typedef int volatile const x6;
typedef int const volatile x7;

typedef char arr_42[42];
typedef char const arr_c_42[42];
typedef char volatile arr_v_42[42];
typedef char const volatile arr_cv_42[42];
typedef char volatile const arr_vc_42[42];
}

namespace after{
typedef void x1;
typedef incomplete_type x2;
typedef int* x3;
typedef int* volatile x4;
typedef void const * x5;
typedef int volatile x6;
typedef int volatile x7;

typedef char arr_42[42];
typedef char arr_c_42[42];
typedef char volatile arr_v_42[42];
typedef char volatile arr_cv_42[42];
typedef char volatile arr_vc_42[42];
} }

namespace is_volatile{
namespace yes{

typedef void * volatile vvoid_ptr_t;
typedef volatile int volatile_int_t;
typedef int* volatile const int_volatile_const_t;
typedef int* const volatile int_const_volatile_t;
}

namespace no{
typedef void volatile * void_ptr_to_v_t;
typedef int* int_ptr_t;
typedef const int* const_int_ptr_t;
typedef int* volatile_int_ptr_t;
Expand All @@ -463,12 +478,30 @@ namespace before{
typedef void * volatile x1;
typedef volatile int x2;
typedef int* x3;
typedef void volatile * x4;
typedef int volatile const x5;
typedef int const volatile x6;

typedef char arr_42[42];
typedef char const arr_c_42[42];
typedef char volatile arr_v_42[42];
typedef char const volatile arr_cv_42[42];
typedef char volatile const arr_vc_42[42];
}

namespace after{
typedef void * x1;
typedef int x2;
typedef int* x3;
typedef void volatile * x4;
typedef int const x5;
typedef int const x6;

typedef char arr_42[42];
typedef char const arr_c_42[42];
typedef char arr_v_42[42];
typedef char const arr_cv_42[42];
typedef char const arr_vc_42[42];
} }


Expand All @@ -488,6 +521,12 @@ namespace before{
typedef int* const x32;

typedef void(*x40)();

typedef char arr_42[42];
typedef char const arr_c_42[42];
typedef char volatile arr_v_42[42];
typedef char const volatile arr_cv_42[42];
typedef char volatile const arr_vc_42[42];
}

namespace after{
Expand All @@ -504,6 +543,12 @@ namespace after{
typedef int* x32;

typedef void(*x40)();

typedef char arr_42[42];
typedef char arr_c_42[42];
typedef char arr_v_42[42];
typedef char arr_cv_42[42];
typedef char arr_vc_42[42];
} }


Expand Down Expand Up @@ -548,9 +593,6 @@ namespace yes{

namespace no{

typedef details::const_item const_item;
typedef details::const_container const_container;

class y{
private:
y(){}
Expand All @@ -567,6 +609,9 @@ namespace no{
public:
static singleton_t* instance();
};

typedef details::const_item const_item;
typedef details::const_container const_container;
} }

namespace has_public_constructor{
Expand Down Expand Up @@ -663,6 +708,7 @@ namespace yes{
const int yes2[2] = {0};
const volatile int yes3[2] = {0};
int yes4[2][3];
int const yes5[2] = {0};
}

namespace no{
Expand Down
Loading