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

Skylion007 · 2021-09-09T15:18:59Z

Description

Ensure we don't use const return types since they are useless and can prevent useful optimizations (const references are allowed to be returned though).
Fix some bad templating code that allowed

The annoying edge case to account for here is that our Eigen cast actually has a different cast depending on whether the Eigen array is const (it controls the writability flag). For that reason, we cannot ban const return types from the caster in case the user does similarly off spec things. I have however added the NOLINTS and ensured that all other errant const return types (such as const handles) have been removed.
Note: remove_cv_t will not strip the constness of references :)

Suggested changelog entry:

* Enable readability clang-tidy-const-return and remove useless consts

…g-tidy-const-return

henryiii · 2021-09-09T15:29:49Z

include/pybind11/pybind11.h

-iterator make_key_iterator(Iterator first, Sentinel last, Extra &&... extra) {
+iterator make_key_iterator(Iterator first, Sentinel last, Extra &&...extra) {


Intentional?

Clang-Format. I can revert.

henryiii · 2021-09-09T15:30:50Z

tests/test_eigen.cpp

@@ -244,6 +245,7 @@ TEST_SUBMODULE(eigen, m) {

    // test_fixed, and various other tests
    m.def("fixed_r", [mat]() -> FixedMatrixR { return FixedMatrixR(mat); });
+    // NOLINTNEXTLINE(readability-const-return-type)
    m.def("fixed_r_const", [mat]() -> const FixedMatrixR { return FixedMatrixR(mat); });


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

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

How annoyingly offspec, Fixing this.

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

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

rwgk · 2021-09-09T16:39:17Z

tests/test_eigen.cpp

@@ -244,6 +245,7 @@ TEST_SUBMODULE(eigen, m) {

    // test_fixed, and various other tests
    m.def("fixed_r", [mat]() -> FixedMatrixR { return FixedMatrixR(mat); });
+    // NOLINTNEXTLINE(readability-const-return-type)
    m.def("fixed_r_const", [mat]() -> const FixedMatrixR { return FixedMatrixR(mat); });


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

rwgk

Thanks for the Eigen comment! That makes it totally clear.

rwgk · 2021-09-10T04:27:26Z

I'll merge this now, to then merge into to smart_holder branch, cleanup there, and then run the Google global testing overnight.

Skylion007 · 2021-09-10T04:36:40Z

Oh darn, I didn't merge this yes because there was a useless a NOLINT I hadn't got around to deleting.

rwgk · 2021-09-10T04:40:13Z

Oops, sorry. Is that a one-line delete? Where? I could test it via the smart_holder testing, we could simply push directly to master.

Skylion007 · 2021-09-10T04:41:05Z

cast.h 1187

rwgk · 2021-09-10T04:43:34Z

Thanks! I still have everything open (local clang-tidy testing). I'll take care of it.

rwgk · 2021-09-10T04:56:36Z

I'm running into a snag with my grand overnight testing plan. See below.
I'm too tired right now to debug. I'll work on this tomorrow. I'll hold off removing the NOLINT until I understand the error.

ERROR: /google/src/cloud/rwgk/pybind11_sh_update/google3/third_party/pybind11_protobuf/BUILD:9:15: Compiling third_party/pybind11_protobuf/proto_utils.cc failed: (Exit 1) wrapped_clang failed: error executing command third_party/crosstool/v18/stable/toolchain/bin/wrapped_clang '-frandom-seed=blaze-out/k8-asan-fastbuild/bin/third_party/pybind11_protobuf/_objs/proto_utils/proto_utils.pic.o' -DABSL_ALLOCATOR_NOTHROW ... (remaining 260 arguments skipped).  [forge_remote_host=ixdf7]
third_party/pybind11_protobuf/proto_utils.cc:520:16: error: non-const lvalue reference to type 'pair<...>' cannot bind to a temporary of type 'pair<...>'
    for (auto& item : values) SetItem(item.first, item.second);
               ^    ~
third_party/pybind11/include/pybind11/pytypes.h:1470:27: note: selected 'begin' function with iterator type 'detail::dict_iterator' (aka 'generic_iterator<iterator_policies::dict_readonly>')
    detail::dict_iterator begin() const { return {*this, 0}; }
                          ^
third_party/pybind11_protobuf/proto_utils.cc:1072:14: error: non-const lvalue reference to type 'pair<...>' cannot bind to a temporary of type 'pair<...>'
  for (auto& item : kwargs_in) {
             ^    ~
third_party/pybind11/include/pybind11/pytypes.h:1470:27: note: selected 'begin' function with iterator type 'detail::dict_iterator' (aka 'generic_iterator<iterator_policies::dict_readonly>')
    detail::dict_iterator begin() const { return {*this, 0}; }
                          ^
2 errors generated.

rwgk · 2021-09-10T05:11:15Z

Never mind, a quick guess worked: the proto_utils.cc code needs to be changed to const auto& in the two places pointed to in the error message.
Clearly your clang-tidy change is doing something good already for code health beyond pybind11 core :-)

rwgk · 2021-09-10T15:44:55Z

I actually managed to run the Google global testing overnight. There is a pretty high number of failures (hundreds), with estimated 9 root causes. Unfortunately I cannot share the full list publicly, but one root cause points to pytorch. I'll work on this asap and will log my updates here.

…sts reflective of user code that breaks when those `const` are removed.

* Restoring `const` removed from pytypes.h in PR #3254, adding tests reflective of user code that breaks when those `const` are removed. * clang-tidy NOLINTs (and one collateral fix). * Inserting PYBIND11_CONST_FOR_STRICT_PLATFORMS * Trying `defined(__APPLE__)` * Trying again: `auto it` for strict platforms. * Adding NOLINTNEXTLINE(bugprone-macro-parentheses), expanding comments. * Labeling all changes with `PR #3263`, for easy reference, and to make it easy to undo these changes if we decide to do so in the future.

Skylion007 added 2 commits September 9, 2021 11:12

Enable clang-tidy readability-const-return

b542f27

Merge branch 'master' of https://github.com/pybind/pybind11 into clan…

de74809

…g-tidy-const-return

Skylion007 assigned rwgk Sep 9, 2021

Skylion007 requested a review from henryiii as a code owner September 9, 2021 15:18

Skylion007 assigned henryiii Sep 9, 2021

Skylion007 changed the title ~~Clang tidy const return~~ maint(Clang-Tidy):readability-const-return Sep 9, 2021

Skylion007 changed the title ~~maint(Clang-Tidy):readability-const-return~~ maint(Clang-Tidy): readability-const-return Sep 9, 2021

henryiii reviewed Sep 9, 2021

View reviewed changes

PyTest functional

63bf253

henryiii reviewed Sep 9, 2021

View reviewed changes

Skylion007 added 3 commits September 9, 2021 11:44

Fix regression

8fef23c

Fix actual regression

3a9516f

Remove one more NOLINT

3940960

rwgk approved these changes Sep 9, 2021

View reviewed changes

Update comment

2874d74

rwgk approved these changes Sep 9, 2021

View reviewed changes

Skylion007 requested a review from henryiii September 9, 2021 18:06

henryiii approved these changes Sep 9, 2021

View reviewed changes

rwgk merged commit ae07d4c into pybind:master Sep 10, 2021

github-actions bot added the needs changelog Possibly needs a changelog entry label Sep 10, 2021

rwgk mentioned this pull request Sep 10, 2021

Fixing NOLINT mishap #3260

Merged

rwgk added a commit to rwgk/pybind11 that referenced this pull request Sep 10, 2021

Restoring const removed from pytypes.h in PR pybind#3254, adding te…

551b185

…sts reflective of user code that breaks when those `const` are removed.

rwgk mentioned this pull request Sep 10, 2021

Follow-on to PR #3254, to address user code breakages. #3263

Merged

henryiii removed the needs changelog Possibly needs a changelog entry label Sep 16, 2021

rwgk mentioned this pull request Feb 10, 2023

FWD pybind11 google/pybind11clif#3254

Closed

		iterator make_key_iterator(Iterator first, Sentinel last, Extra &&... extra) {
		iterator make_key_iterator(Iterator first, Sentinel last, Extra &&...extra) {

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

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

Uh oh!

Conversation

Skylion007 commented Sep 9, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Suggested changelog entry:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rwgk left a comment

Choose a reason for hiding this comment

Uh oh!

rwgk commented Sep 10, 2021

Uh oh!

Skylion007 commented Sep 10, 2021

Uh oh!

rwgk commented Sep 10, 2021

Uh oh!

Skylion007 commented Sep 10, 2021

Uh oh!

rwgk commented Sep 10, 2021

Uh oh!

rwgk commented Sep 10, 2021

Uh oh!

rwgk commented Sep 10, 2021

Uh oh!

rwgk commented Sep 10, 2021

Uh oh!

Uh oh!

Skylion007 commented Sep 9, 2021 •

edited

Loading