Skip to content

maint(Clang-Tidy): readability-const-return #3254

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

Merged
merged 7 commits into from
Sep 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion .clang-tidy
Original file line number Diff line number Diff line change
Expand Up @@ -31,9 +31,10 @@ modernize-use-override,
modernize-use-using,
*performance*,
readability-avoid-const-params-in-decls,
readability-const-return-type,
readability-container-size-empty,
readability-else-after-return,
readability-delete-null-pointer,
readability-else-after-return,
readability-implicit-bool-conversion,
readability-make-member-function-const,
readability-misplaced-array-index,
Expand Down
7 changes: 5 additions & 2 deletions include/pybind11/cast.h
Original file line number Diff line number Diff line change
Expand Up @@ -1178,13 +1178,15 @@ class argument_loader {
}

template <typename Return, typename Guard, typename Func>
// NOLINTNEXTLINE(readability-const-return-type)
enable_if_t<!std::is_void<Return>::value, Return> call(Func &&f) && {
return std::move(*this).template call_impl<Return>(std::forward<Func>(f), indices{}, Guard{});
return std::move(*this).template call_impl<remove_cv_t<Return>>(std::forward<Func>(f), indices{}, Guard{});
}

template <typename Return, typename Guard, typename Func>
// NOLINTNEXTLINE(readability-const-return-type)
enable_if_t<std::is_void<Return>::value, void_type> call(Func &&f) && {
std::move(*this).template call_impl<Return>(std::forward<Func>(f), indices{}, Guard{});
std::move(*this).template call_impl<remove_cv_t<Return>>(std::forward<Func>(f), indices{}, Guard{});
return void_type();
}

Expand All @@ -1206,6 +1208,7 @@ class argument_loader {
}

template <typename Return, typename Func, size_t... Is, typename Guard>
// NOLINTNEXTLINE(readability-const-return-type)
Return call_impl(Func &&f, index_sequence<Is...>, Guard &&) && {
return std::forward<Func>(f)(cast_op<Args>(std::move(std::get<Is>(argcasters)))...);
}
Expand Down
3 changes: 2 additions & 1 deletion include/pybind11/detail/descr.h
Original file line number Diff line number Diff line change
Expand Up @@ -77,7 +77,8 @@ constexpr enable_if_t<B, T1> _(const T1 &d, const T2 &) { return d; }
template <bool B, typename T1, typename T2>
constexpr enable_if_t<!B, T2> _(const T1 &, const T2 &d) { return d; }

template <size_t Size> auto constexpr _() -> decltype(int_to_str<Size / 10, Size % 10>::digits) {
template <size_t Size>
auto constexpr _() -> remove_cv_t<decltype(int_to_str<Size / 10, Size % 10>::digits)> {
return int_to_str<Size / 10, Size % 10>::digits;
}

Expand Down
4 changes: 2 additions & 2 deletions include/pybind11/pybind11.h
Original file line number Diff line number Diff line change
Expand Up @@ -2002,13 +2002,13 @@ template <return_value_policy Policy = return_value_policy::reference_internal,
typename KeyType = decltype((*std::declval<Iterator>()).first),
#endif
typename... Extra>
iterator make_key_iterator(Iterator first, Sentinel last, Extra &&... extra) {
iterator make_key_iterator(Iterator first, Sentinel last, Extra &&...extra) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Intentional?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Clang-Format. I can revert.

using state = detail::iterator_state<Iterator, Sentinel, true, Policy>;

if (!detail::get_type_info(typeid(state), false)) {
class_<state>(handle(), "iterator", pybind11::module_local())
.def("__iter__", [](state &s) -> state& { return s; })
.def("__next__", [](state &s) -> KeyType {
.def("__next__", [](state &s) -> detail::remove_cv_t<KeyType> {
if (!s.first_or_done)
++s.it;
else
Expand Down
6 changes: 3 additions & 3 deletions include/pybind11/pytypes.h
Original file line number Diff line number Diff line change
Expand Up @@ -773,7 +773,7 @@ class sequence_fast_readonly {
protected:
using iterator_category = std::random_access_iterator_tag;
using value_type = handle;
using reference = const handle;
using reference = handle;
using pointer = arrow_proxy<const handle>;

sequence_fast_readonly(handle obj, ssize_t n) : ptr(PySequence_Fast_ITEMS(obj.ptr()) + n) { }
Expand Down Expand Up @@ -816,7 +816,7 @@ class dict_readonly {
protected:
using iterator_category = std::forward_iterator_tag;
using value_type = std::pair<handle, handle>;
using reference = const value_type;
using reference = value_type;
using pointer = arrow_proxy<const value_type>;

dict_readonly() = default;
Expand Down Expand Up @@ -966,7 +966,7 @@ class iterator : public object {
using iterator_category = std::input_iterator_tag;
using difference_type = ssize_t;
using value_type = handle;
using reference = const handle;
using reference = handle;
using pointer = const handle *;

PYBIND11_OBJECT_DEFAULT(iterator, object, PyIter_Check)
Expand Down
4 changes: 4 additions & 0 deletions tests/test_eigen.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -178,6 +178,7 @@ TEST_SUBMODULE(eigen, m) {
ReturnTester() { print_created(this); }
~ReturnTester() { print_destroyed(this); }
static Eigen::MatrixXd create() { return Eigen::MatrixXd::Ones(10, 10); }
// NOLINTNEXTLINE(readability-const-return-type)
static const Eigen::MatrixXd createConst() { return Eigen::MatrixXd::Ones(10, 10); }
Eigen::MatrixXd &get() { return mat; }
Eigen::MatrixXd *getPtr() { return &mat; }
Expand Down Expand Up @@ -244,6 +245,9 @@ TEST_SUBMODULE(eigen, m) {

// test_fixed, and various other tests
m.def("fixed_r", [mat]() -> FixedMatrixR { return FixedMatrixR(mat); });
// Our Eigen does a hack which respects constness through the numpy writeable flag.
// Therefore, the const return actually affects this type despite being an rvalue.
// NOLINTNEXTLINE(readability-const-return-type)
m.def("fixed_r_const", [mat]() -> const FixedMatrixR { return FixedMatrixR(mat); });
Copy link
Collaborator

Choose a reason for hiding this comment

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

This used to return a non-writable numpy array, now it returns a writable one.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Unchanged files with check annotations (Beta) - very nice to see!

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

How annoyingly offspec, Fixing this.

Copy link
Collaborator

Choose a reason for hiding this comment

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

It's a kind-of-handy and "cute" trick, but not at all "normal" C++!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is that trick explained somewhere? (I still don't know what it is.) A one-line comment here with a hint would be nice.

m.def("fixed_c", [mat]() -> FixedMatrixC { return FixedMatrixC(mat); });
m.def("fixed_copy_r", [](const FixedMatrixR &m) -> FixedMatrixR { return m; });
Expand Down