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

Avoid const return type in functions #15671

Merged
merged 2 commits into from Jul 7, 2023

Conversation

masterleinad
Copy link
Member

const doesn't do anything when we are returning values.

Comment on lines 78 to 80
template <typename type>
const type
type
entry(const std::string &name) const;
Copy link
Member

Choose a reason for hiding this comment

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

Could type be a reference/pointer? And if so, would that make the pointer const, or the pointed to object const?

Copy link
Member Author

Choose a reason for hiding this comment

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

The const doesn't matter in that case either, see https://godbolt.org/z/8jesMca3a. Returning a constant pointer is the same as returning a non-constant pointer and a pointer to a constant type is still returned as such.

@bangerth
Copy link
Member

bangerth commented Jul 7, 2023

Several of the testers fail, at least one of them with this error message:

/home/runner/work/dealii/dealii/source/base/mpi.cc:681:5: error: ambiguating new declaration of ‘const std::vector<unsigned int> dealii::Utilities::MPI::mpi_processes_within_communicator(MPI_Comm, MPI_Comm)’
  681 |     mpi_processes_within_communicator(const MPI_Comm, const MPI_Comm)
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from /home/runner/work/dealii/dealii/source/base/mpi.cc:19:
/home/runner/work/dealii/dealii/include/deal.II/base/mpi.h:158:5: note: old declaration ‘std::vector<unsigned int> dealii::Utilities::MPI::mpi_processes_within_communicator(MPI_Comm, MPI_Comm)’
  158 |     mpi_processes_within_communicator(const MPI_Comm comm_large,
      |     ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@bangerth
Copy link
Member

bangerth commented Jul 7, 2023

Still fails:

/jenkins/workspace/dealii-serial_PR-15671/tests/multigrid/mg_data_out_01.cc: In instantiation of ‘void print(dealii::DataOut<dim>&, const dealii::Triangulation<dim, dim>&) [with int dim = 2]’:
/jenkins/workspace/dealii-serial_PR-15671/tests/multigrid/mg_data_out_01.cc:62:8:   required from ‘void do_test() [with int dim = 2]’
/jenkins/workspace/dealii-serial_PR-15671/tests/multigrid/mg_data_out_01.cc:95:14:   required from here
/jenkins/workspace/dealii-serial_PR-15671/tests/multigrid/mg_data_out_01.cc:34:9: error: cannot bind non-const lvalue reference of type ‘std::pair<std::function<dealii::TriaIterator<dealii::CellAccessor<2, 2> >(const dealii::Triangulation<2, 2>&)>, std::function<dealii::TriaIterator<dealii::CellAccessor<2, 2> >(const dealii::Triangulation<2, 2>&, const dealii::TriaIterator<dealii::CellAccessor<2, 2> >&)> >&’ to an rvalue of type ‘std::pair<std::function<dealii::TriaIterator<dealii::CellAccessor<2, 2> >(const dealii::Triangulation<2, 2>&)>, std::function<dealii::TriaIterator<dealii::CellAccessor<2, 2> >(const dealii::Triangulation<2, 2>&, const dealii::TriaIterator<dealii::CellAccessor<2, 2> >&)> >’
   34 |   auto &p = data_out.get_cell_selection();
      |         ^
/jenkins/workspace/dealii-serial_PR-15671/tests/multigrid/mg_data_out_01.cc: In instantiation of ‘void print(dealii::DataOut<dim>&, const dealii::Triangulation<dim, dim>&) [with int dim = 3]’:
/jenkins/workspace/dealii-serial_PR-15671/tests/multigrid/mg_data_out_01.cc:62:8:   required from ‘void do_test() [with int dim = 3]’
/jenkins/workspace/dealii-serial_PR-15671/tests/multigrid/mg_data_out_01.cc:96:14:   required from here
/jenkins/workspace/dealii-serial_PR-15671/tests/multigrid/mg_data_out_01.cc:34:9: error: cannot bind non-const lvalue reference of type ‘std::pair<std::function<dealii::TriaIterator<dealii::CellAccessor<3, 3> >(const dealii::Triangulation<3, 3>&)>, std::function<dealii::TriaIterator<dealii::CellAccessor<3, 3> >(const dealii::Triangulation<3, 3>&, const dealii::TriaIterator<dealii::CellAccessor<3, 3> >&)> >&’ to an rvalue of type ‘std::pair<std::function<dealii::TriaIterator<dealii::CellAccessor<3, 3> >(const dealii::Triangulation<3, 3>&)>, std::function<dealii::TriaIterator<dealii::CellAccessor<3, 3> >(const dealii::Triangulation<3, 3>&, const dealii::TriaIterator<dealii::CellAccessor<3, 3> >&)> >’
make[3]: *** [CMakeFiles/multigrid.mg_data_out_01.debug.dir/build.make:76: CMakeFiles/multigrid.mg_data_out_01.debug.dir/mg_data_out_01.cc.o] Error 1
make[2]: *** [CMakeFiles/Makefile2:1902: CMakeFiles/multigrid.mg_data_out_01.debug.dir/all] Error 2
make[1]: *** [CMakeFiles/Makefile2:2143: CMakeFiles/multigrid.mg_data_out_01.debug.test.dir/rule] Error 2

@bangerth
Copy link
Member

bangerth commented Jul 7, 2023

That is a pretty fascinating failure, by the way. It shows that the const on an rvalue return object really does matter in some contexts!

@bangerth
Copy link
Member

bangerth commented Jul 7, 2023

In short, this does not compile:

const int f();

int &i = f();

The error in the test probably points to a case where we shouldn't store a reference to begin with.

@masterleinad
Copy link
Member Author

That is a pretty fascinating failure, by the way. It shows that the const on an rvalue return object really does matter in some contexts!

Yes, also see https://godbolt.org/z/r5nhc6Yde. auto& becomes const Foo& when the return type is const.

@bangerth bangerth merged commit 6bf41e8 into dealii:master Jul 7, 2023
15 checks passed
@bangerth
Copy link
Member

bangerth commented Jul 8, 2023

I wonder whether we want to reconsider this. It breaks ASPECT in ~4 places: geodynamics/aspect#5208

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants