NCAR / lrose-core

Core C/C++ code for LROSE.
https://www.eol.ucar.edu/content/lidar-radar-open-software-environment
Other
92 stars 51 forks source link

RadxBeamBlock C++ error #57

Closed mcbdroid closed 5 years ago

mcbdroid commented 6 years ago

Hi,

I am trying to build just the Radx package of LROSE, using ./build/checkout_and_build_auto.py --debug --prefix /usr/local/lrose --package radx.

After a couple of minor C++ fixes to array.h and array_utils.h (replacing has_trivial_copy_constructor with is_trivially_copyable), I am getting a C++ error on metadata.cc indicating that I've exceed the limit on template instantiation depth and that I should increase it using -ftemplate-depth=n (where n should be greater than 1024 I presume which is the C++ 11 limit?)

Which script/file should I set the '-ftemplate-depth' flag? Or is there possibly another work-around?

Thanks very much, Mark

mcbdroid commented 6 years ago

Sorry, forgot system and compiler details:

Ubuntu 18.04

$ gcc --version gcc (Ubuntu 7.3.0-16ubuntu3) 7.3.0

$ g++ --version g++ (Ubuntu 7.3.0-16ubuntu3) 7.3.0

marack commented 6 years ago

Hi Mark,

I'm the original author of the beam blocking code. This was written a few years ago and conditional selection between std::copy and std::memset was really a case of premature optimization. The std::copy implementation should have been all we needed (see the top answer at https://stackoverflow.com/questions/4707012/is-it-better-to-use-stdmemcpy-or-stdcopy-in-terms-to-performance).

Since we know the array template and array utils are only used with trivial types in RadxBeamBlock you could safely just remove the unnecessary template magic:

If these are what is causing the template overflow in metadata.cc then that should fix it. There seems to be another copy of this code for optical flow which would benefit from the same simplification.

mcbdroid commented 6 years ago

Hi Mark,

Many thanks for the reply. I'm not a C++ guy, so am going to pass by my edits for your verification:

  1. In array_utils.h: Replaced: copy_array(input.data(), output.size(), output.data()); With: std::copy(input.data(), output.size(), output.data());

  2. In array.h: Looks like there are 4 copy_array templates? Commented those out.

  3. Not sure how to "remove" the std::enable_if in your third bullet above, if you could advise I'd appreciate it.

Thanks, Mark

On Mon, Aug 6, 2018 at 10:47 PM, Mark Curtis notifications@github.com wrote:

Hi Mark,

I'm the original author of the beam blocking code. This was written a few years ago and conditional selection between std::copy and std::memset was really a case of premature optimization. The std::copy implementation should have been all we needed (see the top answer at https://stackoverflow.com/questions/4707012/is-it- better-to-use-stdmemcpy-or-stdcopy-in-terms-to-performance).

Since we know the array template and array utils are only used with trivial types in RadxBeamBlock you could safely just remove the unnecessary template magic:

  • Replace the call to copy_array in array_utils.h with the std::copy call from that implementation
  • Remove the copy_array function templates
  • Remove the std::enable_if condition on the zero function template in array utils

If these are what is causing the template overflow in metadata.cc then that should fix it. There seems to be another copy of this code for optical flow which would benefit from the same simplification.

— You are receiving this because you modified the open/close state. Reply to this email directly, view it on GitHub https://github.com/NCAR/lrose-core/issues/57#issuecomment-410915640, or mute the thread https://github.com/notifications/unsubscribe-auth/AFdM7o5lNgAoz4vsZxbSB5oetjk36_3iks5uOP_PgaJpZM4VxMdZ .

marack commented 6 years ago

Hi Mark,

For (1) that's not quite it. You should replace copy_array(input.data(), output.size(), output.data()); in array_utils.h with std::copy(input.data(), input.data() + output.size(), output.data()).

For (2) you should delete the first two copy_array (these are where the function is declared). The second two need to be replaced like you do in (1) since these are places where the function is being called. Similar to above they would change to std::copy(rhs.data_.get(), rhs.data_.get() + size_, data.get()).

For (3) you would change this:

template <typename T, typename = typename std::enable_if<std::has_trivial_copy_constructor<typename T::value_type>::value>::type>
T& zero(T& output)

to

template <typename T>
T& zero(T& output)

I'm not sure that these changes will resolve the problem you initially reported but it's worth a try. If they don't, could you paste in your actual compile error from metadata.cc and I'll see if something jumps out?

mcbdroid commented 6 years ago

Hi Mark,

I implemented the changes per your last email. Seems like that fixed the issues with array.h and arrah_utils.h, but now getting a problem in metadata.cc and variant.h:

metadata.cc:37:71: required from here variant.h:100:59: fatal error: template instantiation depth exceeds maximum of 900 (use -ftemplate-depth= to increase the maximum) variant(S&& value) noexcept(noexcept(S(std::forward(value))))


compilation terminated.
makefile:511: recipe for target 'metadata.o' failed
make[3]: *** [metadata.o] Error 1
make[3]: Leaving directory
'/tmp/lrose_build/lrose-core/codebase/apps/Radx/src/RadxBeamBlock'

Any further help would be appreciated.

Thanks,
Mark

On Tue, Aug 7, 2018 at 6:54 PM, Mark Curtis <notifications@github.com>
wrote:

> Hi Mark,
>
> For (1) that's not quite it. You should replace copy_array(input.data(),
> output.size(), output.data()); in array_utils.h with std::copy(input.data(),
> input.data() + output.size(), output.data()).
>
> For (2) you should delete the first two copy_array (these are where the
> function is declared). The second two need to be replaced like you do in
> (1) since these are places where the function is being called. Similar to
> above they would change to std::copy(rhs.data_.get(), rhs.data_.get() +
> size_, data.get()).
>
> For (3) you would change this:
>
> template <typename T, typename = typename std::enable_if<std::has_trivial_copy_constructor<typename T::value_type>::value>::type>
> T& zero(T& output)
>
> to
>
> template <typename T>
> T& zero(T& output)
>
> I'm not sure that these changes will resolve the problem you initially
> reported but it's worth a try. If they don't, could you paste in your
> actual compile error from metadata.cc and I'll see if something jumps out?
>
> —
> You are receiving this because you modified the open/close state.
> Reply to this email directly, view it on GitHub
> <https://github.com/NCAR/lrose-core/issues/57#issuecomment-411228559>, or mute
> the thread
> <https://github.com/notifications/unsubscribe-auth/AFdM7iHSc_UrQ5A2UfSf5xZk-9zEY1Ycks5uOhqxgaJpZM4VxMdZ>
> .
>
mcbdroid commented 6 years ago

Hi Mark,

I have returned to trying to build lrose from src. I still am having problems as referenced in my 8/13 email above. Any thoughts and/or suggestions?

Thanks, Mark ASRC

On Mon, Aug 13, 2018 at 12:11 PM Mark Beauharnois mcbdroid@gmail.com wrote:

Hi Mark,

I implemented the changes per your last email. Seems like that fixed the issues with array.h and arrah_utils.h, but now getting a problem in metadata.cc and variant.h:

metadata.cc:37:71: required from here variant.h:100:59: fatal error: template instantiation depth exceeds maximum of 900 (use -ftemplate-depth= to increase the maximum) variant(S&& value) noexcept(noexcept(S(std::forward(value))))


compilation terminated.
makefile:511: recipe for target 'metadata.o' failed
make[3]: *** [metadata.o] Error 1
make[3]: Leaving directory
'/tmp/lrose_build/lrose-core/codebase/apps/Radx/src/RadxBeamBlock'

Any further help would be appreciated.

Thanks,
Mark

On Tue, Aug 7, 2018 at 6:54 PM, Mark Curtis <notifications@github.com>
wrote:

> Hi Mark,
>
> For (1) that's not quite it. You should replace copy_array(input.data(),
> output.size(), output.data()); in array_utils.h with std::copy(input.data(),
> input.data() + output.size(), output.data()).
>
> For (2) you should delete the first two copy_array (these are where the
> function is declared). The second two need to be replaced like you do in
> (1) since these are places where the function is being called. Similar to
> above they would change to std::copy(rhs.data_.get(), rhs.data_.get() +
> size_, data.get()).
>
> For (3) you would change this:
>
> template <typename T, typename = typename std::enable_if<std::has_trivial_copy_constructor<typename T::value_type>::value>::type>
> T& zero(T& output)
>
> to
>
> template <typename T>
> T& zero(T& output)
>
> I'm not sure that these changes will resolve the problem you initially
> reported but it's worth a try. If they don't, could you paste in your
> actual compile error from metadata.cc and I'll see if something jumps
> out?
>
> —
> You are receiving this because you modified the open/close state.
> Reply to this email directly, view it on GitHub
> <https://github.com/NCAR/lrose-core/issues/57#issuecomment-411228559>,
> or mute the thread
> <https://github.com/notifications/unsubscribe-auth/AFdM7iHSc_UrQ5A2UfSf5xZk-9zEY1Ycks5uOhqxgaJpZM4VxMdZ>
> .
>