Skip to content
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.

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

Use using instead of typedef [kdtree, keypoints, octree] #3122

Conversation

SunBlack
Copy link
Contributor

@SunBlack SunBlack commented Jun 5, 2019

Changes are done by: run-clang-tidy -header-filter='.' -checks='-,modernize-use-using' -fix

@@ -1669,7 +1669,7 @@ pcl::keypoints::brisk::Layer::halfsample (
// compute horizontal pairwise average and store
p_dest_char = reinterpret_cast<unsigned char*> (p_dest);
#ifdef __GNUC__
typedef unsigned char __attribute__ ((__may_alias__)) UCHAR_ALIAS;
using UCHAR_ALIAS = unsigned char;
Copy link
Member

Choose a reason for hiding this comment

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

Any idea what did this attribute do?

Copy link
Member

@SergioRAgostinho SergioRAgostinho Jun 5, 2019

Choose a reason for hiding this comment

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

I have the feeling it likely disables aliasing warnings that will be triggered by this.

const UCHAR_ALIAS* result = reinterpret_cast<const UCHAR_ALIAS*> (&result1);

More on the topic:
https://stackoverflow.com/questions/6313050/gcc-how-to-use-attribute-may-alias-properly-to-avoid-derefencing-type
https://en.wikipedia.org/wiki/Aliasing_(computing)#Conflicts_with_optimization

Copy link
Member

Choose a reason for hiding this comment

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

Let's maybe leave the typedef here?

Copy link
Member

Choose a reason for hiding this comment

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

I'm surprised the same logic can't be expressed with using.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe it could. But frankly speaking... I have no desire to study this question. Getting rid of each and every typedef is a no-goal for us, so leaving as-is is the safest option I think.

Copy link
Member

@taketwo taketwo Jun 9, 2019

Choose a reason for hiding this comment

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

@SunBlack according to this page, using alias should look like:

using identifier attr(optional) = type-id 

That is, attributes come before equals sign. Right now attributes are after equals and the thing still compiles, however we don't know whether it works as expected. I'd suggest we keep typedef to avoid potential troubles.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You are right, my code is not correct. I played now a bit with it by using a aliasing example from here. I adjusted the code the to use aliasing typedef, see here.

Result:
typedef float FLOAT_ALIAS; and using FLOAT_ALIAS = __attribute__ ((__may_alias__)) float; are producing:

foo(float*, int*): # @foo(float*, int*)
  mov dword ptr [rsi], 1
  mov dword ptr [rdi], 0
  mov eax, 1
  ret

typedef float __attribute__ ((__may_alias__)) FLOAT_ALIAS; and using FLOAT_ALIAS __attribute__ ((__may_alias__)) = float; are producing:

foo(float*, int*): # @foo(float*, int*)
  mov dword ptr [rsi], 1
  mov dword ptr [rdi], 0
  mov eax, dword ptr [rsi]
  ret

@taketwo I believed attr in using identifier attr(optional) = type-id is only for new attributes like [[deprecated]], but are right :).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just to add a note: I don't believe we need aliasing here anywhere, because we are just reading value of result1 (so nothing to optimize there).

Copy link
Member

Choose a reason for hiding this comment

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

👍 thanks for researching assembly, good to know!

@SunBlack SunBlack force-pushed the modernize-use-using_kdtree_keypoints_octree branch from 2f0dc49 to cc5a3bc Compare June 6, 2019 11:13
Copy link
Member

@taketwo taketwo left a comment

Choose a reason for hiding this comment

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

As per inline comment, according to this page, using alias should look like:

using identifier attr(optional) = type-id 

That is, attributes come before equals sign. Right now attributes are after equals and the thing still compiles, however we don't know whether it works as expected. I'd suggest we keep typedef to avoid potential troubles.

@SunBlack SunBlack force-pushed the modernize-use-using_kdtree_keypoints_octree branch from cc5a3bc to 249b5b8 Compare June 12, 2019 11:03
@taketwo taketwo merged commit 308138a into PointCloudLibrary:master Jun 13, 2019
@taketwo taketwo added the c++14 label Jun 13, 2019
@SunBlack SunBlack deleted the modernize-use-using_kdtree_keypoints_octree branch June 13, 2019 23:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants