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

Explicitly use nest namespace to avoid compiler confusion #2253

Merged
merged 2 commits into from
Jan 7, 2022

Conversation

hakonsbm
Copy link
Contributor

@hakonsbm hakonsbm commented Jan 5, 2022

When compiling with -std=c++17, the compiler confuses the apply() in nest.h with std::apply() from <tuple>. This happens with both GCC and Clang. However, there doesn't seem to be any place where we say that the std namespace should be used in that context. The calls are therefore changed to explicitly use the nest namespace, which fixes the problem and therefore fixes #2252.

As a side-note, it's possible to reproduce the behaviour without NEST, with this reproducer.
#include <tuple>
#include <vector>

namespace foo
{

void
bar()
{
}

std::vector< double >
apply( const std::vector< double >&, const std::vector< double >& )
{
  return {};
}

void
run()
{
  std::vector< double > vec;

  bar();                  // OK
  apply( vec, vec );      // Tries to use std::apply() from <tuple>, which causes an error with the arguments
  foo::apply( vec, vec ); // OK
}
} // end of namespace foo

int
main()
{
  foo::run();
}

@hakonsbm hakonsbm added S: Normal Handle this with default priority I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) T: External bug Not an issue that can be solved here. (May need documentation, though) labels Jan 5, 2022
@heplesser
Copy link
Contributor

@hakonsbm Thanks for the quick fix! But some numerics seem to have changed (last bit of float), we probably should change the test to use something like x == pytest.approx(x_expected).

@ikitayama
Copy link

ikitayama commented Jan 6, 2022

@hakonsbm if you like, I am happy to bring your reproducer to the LLVM community's attention. As I am using the main branch of LLVM.

@hakonsbm
Copy link
Contributor Author

hakonsbm commented Jan 6, 2022

@heplesser I've updated the test, and all tests pass now.

@ikitayama Sure, go ahead.

@heplesser
Copy link
Contributor

@hakonsbm if you like, I am happy to bring your reproducer to the LLVM community's attention. As I am using the main branch of LLVM.

Since the problem also occurs with gcc 11 when compiling with -std=c++17, could you also open a ticket on https://gcc.gnu.org/bugzilla?

Copy link
Contributor

@stinebuu stinebuu left a comment

Choose a reason for hiding this comment

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

Looks good to me! 👍

@stinebuu stinebuu merged commit 4fe7a0e into nest:master Jan 7, 2022
@hakonsbm hakonsbm deleted the fix_c++17_namespace_confusion branch January 7, 2022 07:51
@terhorstd terhorstd added T: Maintenance Work to keep up the quality of the code and documentation. and removed T: External bug Not an issue that can be solved here. (May need documentation, though) labels Jan 21, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
I: No breaking change Previously written code will work as before, no one should note anything changing (aside the fix) S: Normal Handle this with default priority T: Maintenance Work to keep up the quality of the code and documentation.
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

master build fails with -std=c++17 Apple gcc flag
5 participants