Fixes for cv-qualified arrays #35

praetorian20 · 2016-03-05T18:13:00Z

I've made changes to some type traits functions to fix detection of cv-qualified arrays. Before these fixes, is_const would return False for T const[N]. The same problem existed for volatile too. This bug is causing a pyplusplus unit test to fail.

To see how such arrays were not being handled correctly, take only the changes to unittests/array_bug_tester.py and unittests/data/type_traits_castxml.hpp and run unit tests.

The PR is not quite ready for merging yet, there are a couple of things I'd like to clarify. I'll comment on those inline.

praetorian20 · 2016-03-05T22:23:27Z

unittests/array_bug_tester.py

+        arr_type = global_ns.variable('arr').type
+        if self.config.xml_generator == "gccxml":
+            self.assertTrue(
+                'char[4] const' == arr_type.decl_string,


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

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.

iMichka · 2016-03-07T22:08:27Z

Hi

thank you very much for the improvements. I will need some time to read through the code. I am not the original author, so I do not know all the code by hearth. Still, I did already look at these methods once for another bug. Some parts already seemed tricky to me, and it is often very difficult to follow what is going on. I'll answer to your comments inline.

iMichka · 2016-03-13T22:33:01Z

pygccxml/declarations/type_traits.py

+                base_type = nake_type.base.base
+            result_type = base_type
+            if is_v:
+                result_type = cpptypes.volatile_t(result_type)


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.

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.

praetorian20 · 2016-03-24T16:55:42Z

I haven't forgotten that I still need to look into this PR a bit. I'll get back on it next week, and hopefully have it ready for merging. If you want to release 1.7.3 before then, I think this can be merged in its current state, things function correctly for CastXML, I just need to look into some odd behavior with GCCXML.

iMichka · 2016-03-24T17:28:30Z

I'll play around with this this weekend.

1.7.3 can wait a little bit, building/releasing does not take too long. It's the last patch I want to get in the 1.7.3, so lets not rush it.

iMichka · 2016-03-31T21:53:47Z

I finally tagged the 1.7.3 release. There are already a lot of patches in there, and I initially wanted to keep it short. I moved this PR to the 1.7.4 milestone. Do not worry I will make a release even for a single PR, so 1.7.4 can go out very fast if needed.

I will need some more time to review this PR anyway, I'll try to find some time this weekend.

praetorian20 · 2016-04-08T23:24:18Z

pygccxml/declarations/type_traits.py

+                result_type = cpptypes.volatile_t(result_type)
+            return cpptypes.array_t(result_type, nake_type.size)
+
+        elif isinstance(nake_type, cpptypes.volatile_t):


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.

iMichka · 2016-04-11T21:59:22Z

I will not have any time in the 2 next days, but I will be able to review the patch more in detail this weekend (just to let you know if I don't answer directly). Thanks for the hard work on this.

praetorian20 · 2016-04-16T00:51:29Z

I think this PR is finally ready for merging (assuming CI passes). If it looks good to you, then I'll squash everything into a single commit.

praetorian20 · 2016-04-16T17:40:54Z

CI failed with the last commit because I hadn't rebased and fixed some of the uses of deprecated attributes. I've fixed that now.

iMichka · 2016-04-21T21:44:27Z

Yes, I changed .type to decl_type (like we have decl_string at some places). I wanted to tell you but forgot to write it here. In the real world there would have been just a DeprecationWarning, the CI tests are more strict by failing on warnings. This is just to make sure they are tackled early on.

I just pulled your branch, I'm sitting some hours in the plane this weekend so I'll have time to review it thoroughly. I want to play a little bit around but everything seems fine at the first glance.

Thank you for your patience and all the hard work.

iMichka · 2016-04-25T19:26:26Z

Ok everything looks fine now. I reviewed the patch during the weekend, and played a little bit around. Once you squashed this, I'll merge it and add it to the 1.7.4 release.

cv-qualified arrays were not being handled correctly by type traits manipulations functions. For instance, 'int const[N]' would not be detected as 'const'. Similar problems existed for volatile qualified arrays too. 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. Prior to CastXML/CastXML#55 being fixed, CastXML would incorrectly generate default constructor definitions for types with const data members. This caused pygccxml to incorrectly flag such types as trivially constructible. These tests have now been fixed, and the tests are being executed conditionally only for GCCXML (which handles them correctly), or, if CastXML is being used, then for xml_output_version >= 1.138. gccxml places cv-qualifiers to the right of the array declarator in the decl_string. For instance, given the declaration 'int const[arr[42]', the decl_string from gccxml is 'int [42] const'. Combined type_traits_castxml.hpp and type_traits_gccxml.hpp into a single file - type_traits.hpp. Change-Id: If5deaecf4cc9ab38cb8b8ac90befa9c62ebd0106

iMichka · 2016-04-26T20:29:51Z

This is now merged to develop, and I cherry-picked the patch to the 1.7.4 branch. I'll make a release this weekend. Thanks for all ! The travis build is green again.

praetorian20 reviewed Mar 5, 2016
View reviewed changes

iMichka added the enhancement label Mar 7, 2016

iMichka added this to the 1.7.3 milestone Mar 7, 2016

iMichka reviewed Mar 13, 2016
View reviewed changes

praetorian20 force-pushed the cv_qual_arrays branch from a382707 to 65f116d Compare March 15, 2016 19:31

iMichka modified the milestones: 1.7.4, 1.7.3 Mar 31, 2016

praetorian20 force-pushed the cv_qual_arrays branch from 65f116d to 5abf782 Compare April 8, 2016 23:18

praetorian20 reviewed Apr 8, 2016
View reviewed changes

praetorian20 mentioned this pull request Apr 11, 2016

Default constructor created for non-default constructible type CastXML/CastXML#55

Closed

praetorian20 force-pushed the cv_qual_arrays branch from 708846e to be5bfd1 Compare April 16, 2016 17:07

praetorian20 force-pushed the cv_qual_arrays branch from be5bfd1 to b714e5b Compare April 26, 2016 03:17

praetorian20 mentioned this pull request Apr 26, 2016

Fix warning in core_class_hierarchy.hpp #49

Merged

iMichka merged commit af0def5 into CastXML:develop Apr 26, 2016

praetorian20 deleted the cv_qual_arrays branch April 26, 2016 19:48

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Fixes for cv-qualified arrays #35

Fixes for cv-qualified arrays #35

Uh oh!

praetorian20 commented Mar 5, 2016

Uh oh!

praetorian20 Mar 5, 2016

Uh oh!

iMichka Mar 7, 2016

Uh oh!

iMichka commented Mar 7, 2016

Uh oh!

iMichka Mar 13, 2016

Uh oh!

praetorian20 Mar 13, 2016

Uh oh!

praetorian20 commented Mar 24, 2016

Uh oh!

iMichka commented Mar 24, 2016

Uh oh!

iMichka commented Mar 31, 2016

Uh oh!

praetorian20 Apr 8, 2016

Uh oh!

iMichka commented Apr 11, 2016

Uh oh!

praetorian20 commented Apr 16, 2016

Uh oh!

praetorian20 commented Apr 16, 2016

Uh oh!

iMichka commented Apr 21, 2016 •

edited

Loading

Uh oh!

iMichka commented Apr 25, 2016

Uh oh!

iMichka commented Apr 26, 2016

Uh oh!

Uh oh!

Fixes for cv-qualified arrays #35

Fixes for cv-qualified arrays #35

Uh oh!

Conversation

praetorian20 commented Mar 5, 2016

Uh oh!

praetorian20 Mar 5, 2016

Choose a reason for hiding this comment

Uh oh!

iMichka Mar 7, 2016

Choose a reason for hiding this comment

Uh oh!

iMichka commented Mar 7, 2016

Uh oh!

iMichka Mar 13, 2016

Choose a reason for hiding this comment

Uh oh!

praetorian20 Mar 13, 2016

Choose a reason for hiding this comment

Uh oh!

praetorian20 commented Mar 24, 2016

Uh oh!

iMichka commented Mar 24, 2016

Uh oh!

iMichka commented Mar 31, 2016

Uh oh!

praetorian20 Apr 8, 2016

Choose a reason for hiding this comment

Uh oh!

iMichka commented Apr 11, 2016

Uh oh!

praetorian20 commented Apr 16, 2016

Uh oh!

praetorian20 commented Apr 16, 2016

Uh oh!

iMichka commented Apr 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

iMichka commented Apr 25, 2016

Uh oh!

iMichka commented Apr 26, 2016

Uh oh!

Uh oh!

iMichka commented Apr 21, 2016 •

edited

Loading