Skip to content

fix: allow -Wpedantic in C++20 mode #5322

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.

Sign up for GitHub

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 5 commits into from
Aug 22, 2024
Merged

fix: allow -Wpedantic in C++20 mode #5322

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
1a6fca3
fix: allow -Wpedantic again
henryiii Aug 20, 2024
b0e2ac3
tests: ignore pedantic warning for PYBIND11_DECLARE_HOLDER_TYPE
henryiii Aug 21, 2024
ba93f53
tests: try just turning off pedantic for one file
henryiii Aug 21, 2024
b3d5042
tests: only run pedantic in C++20 mode
henryiii Aug 21, 2024
9ecee59
Update tests/local_bindings.h
henryiii Aug 21, 2024
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
6 changes: 3 additions & 3 deletions docs/advanced/cast/stl.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ the declaration

.. code-block:: cpp

PYBIND11_MAKE_OPAQUE(std::vector<int>);
PYBIND11_MAKE_OPAQUE(std::vector<int>)

before any binding code (e.g. invocations to ``class_::def()``, etc.). This
macro must be specified at the top level (and outside of any namespaces), since
Expand Down Expand Up @@ -207,8 +207,8 @@ The following example showcases usage of :file:`pybind11/stl_bind.h`:
// Don't forget this
#include <pybind11/stl_bind.h>

PYBIND11_MAKE_OPAQUE(std::vector<int>);
PYBIND11_MAKE_OPAQUE(std::map<std::string, double>);
PYBIND11_MAKE_OPAQUE(std::vector<int>)
PYBIND11_MAKE_OPAQUE(std::map<std::string, double>)

// ...

Expand Down
6 changes: 3 additions & 3 deletions docs/advanced/smart_ptrs.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -124,7 +124,7 @@ top namespace level before any binding code:

.. code-block:: cpp

PYBIND11_DECLARE_HOLDER_TYPE(T, SmartPtr<T>);
PYBIND11_DECLARE_HOLDER_TYPE(T, SmartPtr<T>)

The first argument of :func:`PYBIND11_DECLARE_HOLDER_TYPE` should be a
placeholder name that is used as a template parameter of the second argument.
Expand All @@ -136,7 +136,7 @@ by default. Specify

.. code-block:: cpp

PYBIND11_DECLARE_HOLDER_TYPE(T, SmartPtr<T>, true);
PYBIND11_DECLARE_HOLDER_TYPE(T, SmartPtr<T>, true)

if ``SmartPtr<T>`` can always be initialized from a ``T*`` pointer without the
risk of inconsistencies (such as multiple independent ``SmartPtr`` instances
Expand All @@ -154,7 +154,7 @@ specialized:
.. code-block:: cpp

// Always needed for custom holder types
PYBIND11_DECLARE_HOLDER_TYPE(T, SmartPtr<T>);
PYBIND11_DECLARE_HOLDER_TYPE(T, SmartPtr<T>)

// Only needed if the type's `.get()` goes by another name
namespace PYBIND11_NAMESPACE { namespace detail {
Expand Down
3 changes: 3 additions & 0 deletions include/pybind11/detail/common.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -479,6 +479,8 @@ PYBIND11_WARNING_POP
}

\endrst */
PYBIND11_WARNING_PUSH
PYBIND11_WARNING_DISABLE_CLANG("-Wgnu-zero-variadic-macro-arguments")
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just double-checking: is this needed, even with -std=c++20?

Copy link
Collaborator Author

@henryiii henryiii Aug 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Yeah, for some reason Clang produces a warning here. GCC is fine, but Clang complains. It produces a warning saying this is a C++20 feature on newer compilers if set less than C++20, so no idea why the old warning triggers in C++20 mode. I think it's a bug.

#define PYBIND11_MODULE(name, variable, ...) \
static ::pybind11::module_::module_def PYBIND11_CONCAT(pybind11_module_def_, name) \
PYBIND11_MAYBE_UNUSED; \
Expand All @@ -499,6 +501,7 @@ PYBIND11_WARNING_POP
PYBIND11_CATCH_INIT_EXCEPTIONS \
} \
void PYBIND11_CONCAT(pybind11_init_, name)(::pybind11::module_ & (variable))
PYBIND11_WARNING_POP

PYBIND11_NAMESPACE_BEGIN(PYBIND11_NAMESPACE)

Expand Down
3 changes: 3 additions & 0 deletions tests/CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -382,6 +382,9 @@ function(pybind11_enable_warnings target_name)
-Wdeprecated
-Wundef
-Wnon-virtual-dtor)
if(DEFINED CMAKE_CXX_STANDARD AND NOT CMAKE_CXX_STANDARD VERSION_LESS 20)
target_compile_options(${target_name} PRIVATE -Wpedantic)
endif()
endif()

if(PYBIND11_WERROR)
Expand Down
16 changes: 8 additions & 8 deletions tests/local_bindings.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -56,21 +56,21 @@ class LocalSimpleException : public std::exception {
std::string message = "";
};

PYBIND11_MAKE_OPAQUE(LocalVec);
PYBIND11_MAKE_OPAQUE(LocalVec2);
PYBIND11_MAKE_OPAQUE(LocalMap);
PYBIND11_MAKE_OPAQUE(NonLocalVec);
// PYBIND11_MAKE_OPAQUE(NonLocalVec2); // same type as LocalVec2
PYBIND11_MAKE_OPAQUE(NonLocalMap);
PYBIND11_MAKE_OPAQUE(NonLocalMap2);
PYBIND11_MAKE_OPAQUE(LocalVec)
PYBIND11_MAKE_OPAQUE(LocalVec2)
PYBIND11_MAKE_OPAQUE(LocalMap)
PYBIND11_MAKE_OPAQUE(NonLocalVec)
// PYBIND11_MAKE_OPAQUE(NonLocalVec2) // same type as LocalVec2
PYBIND11_MAKE_OPAQUE(NonLocalMap)
PYBIND11_MAKE_OPAQUE(NonLocalMap2)

// Simple bindings (used with the above):
template <typename T, int Adjust = 0, typename... Args>
py::class_<T> bind_local(Args &&...args) {
return py::class_<T>(std::forward<Args>(args)...).def(py::init<int>()).def("get", [](T &i) {
return i.i + Adjust;
});
};
}

// Simulate a foreign library base class (to match the example in the docs):
namespace pets {
Expand Down
2 changes: 1 addition & 1 deletion tests/test_callbacks.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,7 +148,7 @@ TEST_SUBMODULE(callbacks, m) {
m.def("dummy_function2", [](int i, int j) { return i + j; });
m.def(
"roundtrip",
[](std::function<int(int)> f, bool expect_none = false) {
[](std::function<int(int)> f, bool expect_none) {
if (expect_none && f) {
throw std::runtime_error("Expected None to be converted to empty std::function");
}
Expand Down
4 changes: 2 additions & 2 deletions tests/test_eigen_matrix.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,7 +55,7 @@ void reset_refs() {
}

// Returns element 2,1 from a matrix (used to test copy/nocopy)
double get_elem(const Eigen::Ref<const Eigen::MatrixXd> &m) { return m(2, 1); };
double get_elem(const Eigen::Ref<const Eigen::MatrixXd> &m) { return m(2, 1); }

// Returns a matrix with 10*r + 100*c added to each matrix element (to help test that the matrix
// reference is referencing rows/columns correctly).
Expand All @@ -76,7 +76,7 @@ struct CustomOperatorNew {
Eigen::Matrix4d a = Eigen::Matrix4d::Zero();
Eigen::Matrix4d b = Eigen::Matrix4d::Identity();

EIGEN_MAKE_ALIGNED_OPERATOR_NEW;
EIGEN_MAKE_ALIGNED_OPERATOR_NEW
};

TEST_SUBMODULE(eigen_matrix, m) {
Expand Down
2 changes: 1 addition & 1 deletion tests/test_opaque_types.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,7 @@
// This also deliberately doesn't use the below StringList type alias to test
// that MAKE_OPAQUE can handle a type containing a `,`. (The `std::allocator`
// bit is just the default `std::vector` allocator).
PYBIND11_MAKE_OPAQUE(std::vector<std::string, std::allocator<std::string>>);
PYBIND11_MAKE_OPAQUE(std::vector<std::string, std::allocator<std::string>>)

using StringList = std::vector<std::string, std::allocator<std::string>>;

Expand Down
4 changes: 2 additions & 2 deletions tests/test_sequences_and_iterators.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,8 +86,8 @@ class NonCopyableInt {
};
using NonCopyableIntPair = std::pair<NonCopyableInt, NonCopyableInt>;

PYBIND11_MAKE_OPAQUE(std::vector<NonCopyableInt>);
PYBIND11_MAKE_OPAQUE(std::vector<NonCopyableIntPair>);
PYBIND11_MAKE_OPAQUE(std::vector<NonCopyableInt>)
PYBIND11_MAKE_OPAQUE(std::vector<NonCopyableIntPair>)

template <typename PythonType>
py::list test_random_access_iterator(PythonType x) {
Expand Down
15 changes: 9 additions & 6 deletions tests/test_smart_ptr.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@
#include "object.h"
#include "pybind11_tests.h"

// This breaks on PYBIND11_DECLARE_HOLDER_TYPE
PYBIND11_WARNING_DISABLE_GCC("-Wpedantic")

namespace {

// This is just a wrapper around unique_ptr, but with extra fields to deliberately bloat up the
Expand Down Expand Up @@ -279,13 +282,13 @@ struct holder_helper<ref<T>> {
} // namespace PYBIND11_NAMESPACE

// Make pybind aware of the ref-counted wrapper type (s):
PYBIND11_DECLARE_HOLDER_TYPE(T, ref<T>, true);
PYBIND11_DECLARE_HOLDER_TYPE(T, ref<T>, true)
// The following is not required anymore for std::shared_ptr, but it should compile without error:
PYBIND11_DECLARE_HOLDER_TYPE(T, std::shared_ptr<T>);
PYBIND11_DECLARE_HOLDER_TYPE(T, huge_unique_ptr<T>);
PYBIND11_DECLARE_HOLDER_TYPE(T, custom_unique_ptr<T>);
PYBIND11_DECLARE_HOLDER_TYPE(T, shared_ptr_with_addressof_operator<T>);
PYBIND11_DECLARE_HOLDER_TYPE(T, unique_ptr_with_addressof_operator<T>);
PYBIND11_DECLARE_HOLDER_TYPE(T, std::shared_ptr<T>)
PYBIND11_DECLARE_HOLDER_TYPE(T, huge_unique_ptr<T>)
PYBIND11_DECLARE_HOLDER_TYPE(T, custom_unique_ptr<T>)
PYBIND11_DECLARE_HOLDER_TYPE(T, shared_ptr_with_addressof_operator<T>)
PYBIND11_DECLARE_HOLDER_TYPE(T, unique_ptr_with_addressof_operator<T>)

TEST_SUBMODULE(smart_ptr, m) {
// Please do not interleave `struct` and `class` definitions with bindings code,
Expand Down
2 changes: 1 addition & 1 deletion tests/test_stl.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,7 +59,7 @@ struct visit_helper<boost::variant> {
} // namespace PYBIND11_NAMESPACE
#endif

PYBIND11_MAKE_OPAQUE(std::vector<std::string, std::allocator<std::string>>);
PYBIND11_MAKE_OPAQUE(std::vector<std::string, std::allocator<std::string>>)

/// Issue #528: templated constructor
struct TplCtorClass {
Expand Down
2 changes: 1 addition & 1 deletion tests/test_tagbased_polymorphic.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -145,4 +145,4 @@ TEST_SUBMODULE(tagbased_polymorphic, m) {
.def(py::init<std::string>())
.def("purr", &Panther::purr);
m.def("create_zoo", &create_zoo);
};
}
2 changes: 1 addition & 1 deletion tests/test_virtual_functions.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -589,4 +589,4 @@ void initialize_inherited_virtuals(py::module_ &m) {
// Fix issue #1454 (crash when acquiring/releasing GIL on another thread in Python 2.7)
m.def("test_gil", &test_gil);
m.def("test_gil_from_thread", &test_gil_from_thread);
};
}