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

Convert pcl::int_t to std::int_t #3422

Merged
merged 7 commits into from
Oct 21, 2019

Conversation

kunaltyagi
Copy link
Member

Reduces another dependency on boost satisfied by C++14

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.

Thanks, apart from missing deprecation, LGTM.

common/include/pcl/pcl_macros.h Show resolved Hide resolved
@taketwo
Copy link
Member

taketwo commented Oct 18, 2019

There seem to be a problem with my suggestion, we get warnings of this form:

/__w/1/s/common/include/pcl/Vertices.h:19:17: warning: 'using uint32_t = uint32_t' is deprecated: use std::uint32_t instead [-Wdeprecated-declarations]
     std::vector<uint32_t> vertices;

Not sure what is the best course of action. Do we want to force people to always use std:: prefix? If so, we should also use inside the library, right?

@kunaltyagi
Copy link
Member Author

we should also use inside the library, right

Yes. I'll fix those

@kunaltyagi
Copy link
Member Author

kunaltyagi commented Oct 19, 2019

Fixing by using the following script (not exactly, modified)

for type in uint8_t int8_t .... int_fast16_t; do
  for file in grep "[ <(]$type" * -nrl; do
    # had an issue with capture gp in sed
    sed -i "s/ $type/ std::$type/g" $file
    sed -i "s/<$type/[std::$type/g" $file
    sed -i "s/($type/(std::$type/g" $file
    sed -i 's/std::std::/std::/g' $file
  done
done

The change in the last 2 commit will be large. I should have used clang-tidy, but I realized too late.

Since I used sed, some changes will affect whitespace. I haven't checked the diff to see where and will rectify them later

@kunaltyagi kunaltyagi force-pushed the std_int branch 2 times, most recently from 967c527 to ac56411 Compare October 19, 2019 15:19
@kunaltyagi
Copy link
Member Author

PTAL. No more deprecation warning regarding pcl::u*int_.*_t

@taketwo
Copy link
Member

taketwo commented Oct 20, 2019

Thanks, looks good! One thing on my mind though is the deprecation message. As it stands, we get

warning: ‘using uint8_t = uint8_t’ is deprecated: use std::uint8_t instead [-Wdeprecated-declarations]

which is confusing. What do you think about deviating from our usual template and stating explicitly instead of what std::uint8_t should be used? Something like:

warning: ‘using uint8_t = uint8_t’ is deprecated: use std::uint8_t instead of pcl::uint8_t [-Wdeprecated-declarations]

@kunaltyagi
Copy link
Member Author

Sure. That's a good idea actually. Users who use using namespace pcl will see this weird message since the deprecation is in the namespace

@kunaltyagi kunaltyagi merged commit 1e3c62e into PointCloudLibrary:master Oct 21, 2019
@kunaltyagi kunaltyagi deleted the std_int branch October 21, 2019 09:49
@SunBlack
Copy link
Contributor

Just to mention: I'm getting now an compile issue since this PR:

[ 27%] Building CXX object surface/CMakeFiles/pcl_surface.dir/src/gp3.cpp.o
/media/sf_pcl/gpu/surface/src/internal.h(47): error: qualified name is not allowed

/media/sf_pcl/gpu/surface/src/internal.h(52): error: namespace "std" has no member "uint64_type"

/media/sf_pcl/gpu/surface/src/cuda/device.h(74): error: namespace "std" has no member "uint64_type"

/media/sf_pcl/gpu/surface/src/cuda/convex_hull.cu(69): error: namespace "std" has no member "uint64_type"

/media/sf_pcl/gpu/surface/src/cuda/convex_hull.cu(69): error: class "pcl::device::Static<<error-constant>>" has no member "check"

/media/sf_pcl/gpu/surface/src/cuda/convex_hull.cu(265): error: namespace "std" has no member "uint64_type"

/media/sf_pcl/gpu/surface/src/cuda/convex_hull.cu(317): error: namespace "std" has no member "uint64_type"

/media/sf_pcl/gpu/surface/src/cuda/convex_hull.cu(323): error: namespace "std" has no member "uint64_type"

/media/sf_pcl/gpu/surface/src/cuda/convex_hull.cu(337): error: namespace "std" has no member "uint64_type"

/media/sf_pcl/gpu/surface/src/cuda/convex_hull.cu(349): error: no instance of overloaded function "thrust::sort_by_key" matches the argument list
            argument types are: (thrust::device_ptr<<error-type>>, <error-type>, thrust::device_ptr<int>)

/media/sf_pcl/gpu/surface/src/cuda/convex_hull.cu(362): error: namespace "std" has no member "uint64_type"

/media/sf_pcl/gpu/surface/src/cuda/convex_hull.cu(374): error: namespace "std" has no member "uint64_type"

/media/sf_pcl/gpu/surface/src/cuda/convex_hull.cu(375): error: namespace "std" has no member "uint64_type"

/media/sf_pcl/gpu/surface/src/cuda/convex_hull.cu(591): error: namespace "std" has no member "uint64_type"

/media/sf_pcl/gpu/surface/src/cuda/convex_hull.cu(616): error: namespace "std" has no member "uint64_type"

/media/sf_pcl/gpu/surface/src/cuda/convex_hull.cu(692): error: namespace "std" has no member "uint64_type"

/media/sf_pcl/gpu/surface/src/cuda/convex_hull.cu(734): error: namespace "std" has no member "uint64_type"

/media/sf_pcl/gpu/surface/src/cuda/convex_hull.cu(735): error: namespace "std" has no member "uint64_type"

18 errors detected in the compilation of "/tmp/tmpxft_00005dbe_00000000-15_convex_hull.compute_75.cpp1.ii".
CMake Error at pcl_gpu_surface_generated_convex_hull.cu.o.RelWithDebInfo.cmake:279 (message):
  Error generating file

@kunaltyagi
Copy link
Member Author

I see. I probably missed the gpu module compilation (along with CI) since I'm using clang not gcc. It might just be a missing header. I'll send a patch ASAP. Sorry

@SunBlack
Copy link
Contributor

I'm using clang6 to build the PCL ;-). I have already fixed it locally, just waiting until build is fully completed, to be sure there are no further compile issues.

It seems you didn't used best search & replace pattern, as this doesn't look as valid code^^:

pcl::gpu::Static<sizeof(pcl::device::std::uint64_type) == 8>::check();

@kunaltyagi
Copy link
Member Author

kunaltyagi commented Oct 22, 2019

didn't used best search & replace pattern

Yeah. I used uint64_t since my sed threw up when I tried to incorporate \b. I thought I'd catch all compilation issues but apparently not. All the errors are in gpu with uint64_type and that further led to them passing through (grep '\b::u*int[0-9]*_t[a-zA-Z]\+\b' * -nr)

I'm using clang6 to build the PCL ;-). I have already fixed it locally

Oh, thanks a lot. Really sorry. I have clang-9 and gcc-9 and cuda doesn't like that. I switched to cuda-gcc (nvcc + gcc-8) and just started on the compilation issues but got side-tracked by:

Edited out platform-specific troubles.

@SunBlack
Copy link
Contributor

Yeah. I used uint64_t since my sed threw up when I tried to incorporate \b.

Oh you are really using sed? I'm using it on our build server, but I don't really like it. You don't get a hint, that sed e.g. doesn't know \d, so I had to use [0-9] instead. But perl on command line isn't much better (in case you have multiline regular expressions). Don't want to know how much hours I spend in last time for this both tools for an easy task...

@taketwo taketwo changed the title Convert pcl::int_t to std::int_t Convert pcl::int_t to std::int_t Jan 18, 2020
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