-
Notifications
You must be signed in to change notification settings - Fork 165
Fix C++11 build failure by adding compatibility macro for relaxed constexpr (Fixes #1713) #1714
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
base: master
Are you sure you want to change the base?
Conversation
dfd50da to
deedef7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
|
|
||
| constexpr CUDA_HOST_DEVICE array_ref<T>& operator=(const array_ref<T>& a) { | ||
| // FIXED: Loop inside constexpr crashes C++11 | ||
| CLAD_CONSTEXPR_CXX14 CUDA_HOST_DEVICE array_ref<T>& operator=(const array_ref<T>& a) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
CLAD_CONSTEXPR_CXX14 CUDA_HOST_DEVICE array_ref<T>& operator=(const array_ref<T>& a) {
^|
|
||
| /// Return the string representation for the generated derivative. | ||
| constexpr const char* getCode() const { | ||
| CLAD_CONSTEXPR_CXX14 const char* getCode() const { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function 'getCode' should be marked [[nodiscard]] [modernize-use-nodiscard]
| CLAD_CONSTEXPR_CXX14 const char* getCode() const { | |
| [[nodiscard]] CLAD_CONSTEXPR_CXX14 const char* getCode() const { |
Additional context
include/clad/Differentiator/Differentiator.h:36: expanded from macro 'CLAD_CONSTEXPR_CXX14'
#define CLAD_CONSTEXPR_CXX14 constexpr
^| // Enable clad after the header was included. | ||
| // FIXME: The header inclusion should be made automatic if the pragma is seen. | ||
| #pragma clad ON | ||
| #pragma clad ON No newline at end of file |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: unknown pragma ignored [clang-diagnostic-unknown-pragmas]
#pragma clad ON
^
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
deedef7 to
7c134e7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
There were too many comments to post at once. Showing the first 10 out of 91. Check the log or trigger a new build to see more.
|
|
||
| constexpr CUDA_HOST_DEVICE array_ref<T>& operator=(const array_ref<T>& a) { | ||
| CLAD_CONSTEXPR_CXX14 CUDA_HOST_DEVICE array_ref<T>& | ||
| operator=(const array_ref<T>& a) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: operator=() does not handle self-assignment properly [bugprone-unhandled-self-assignment]
operator=(const array_ref<T>& a) {
^| template <> class array_ref<void> { | ||
| private: | ||
| /// The pointer to the underlying array | ||
| void* m_arr = nullptr; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: invalid case style for private member 'm_arr' [readability-identifier-naming]
| void* m_arr = nullptr; | |
| void* m_Arr = nullptr; |
include/clad/Differentiator/ArrayRef.h:243:
- : m_arr((void*)arr), m_size(size) {}
+ : m_Arr((void*)arr), m_size(size) {}include/clad/Differentiator/ArrayRef.h:246:
- : m_arr(other.ptr()), m_size(other.size()) {}
+ : m_Arr(other.ptr()), m_size(other.size()) {}include/clad/Differentiator/ArrayRef.h:250:
- return array_ref<T>((T*)(m_arr), m_size);
+ return array_ref<T>((T*)(m_Arr), m_size);include/clad/Differentiator/ArrayRef.h:252:
- [[nodiscard]] constexpr CUDA_HOST_DEVICE void* ptr() const { return m_arr; }
+ [[nodiscard]] constexpr CUDA_HOST_DEVICE void* ptr() const { return m_Arr; }| /// The pointer to the underlying array | ||
| void* m_arr = nullptr; | ||
| /// The size of the array | ||
| std::size_t m_size = 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: invalid case style for private member 'm_size' [readability-identifier-naming]
| std::size_t m_size = 0; | |
| std::size_t m_Size = 0; |
include/clad/Differentiator/ArrayRef.h:243:
- : m_arr((void*)arr), m_size(size) {}
+ : m_arr((void*)arr), m_Size(size) {}include/clad/Differentiator/ArrayRef.h:246:
- : m_arr(other.ptr()), m_size(other.size()) {}
+ : m_arr(other.ptr()), m_Size(other.size()) {}include/clad/Differentiator/ArrayRef.h:250:
- return array_ref<T>((T*)(m_arr), m_size);
+ return array_ref<T>((T*)(m_arr), m_Size);include/clad/Differentiator/ArrayRef.h:254:
- return m_size;
+ return m_Size;| // delete the default constructor | ||
| array_ref() = delete; | ||
|
|
||
| template <typename T, class = typename std::enable_if< |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use c++14 style type templates [modernize-type-traits]
| template <typename T, class = typename std::enable_if< | |
| template <typename T, class = std::enable_if_t< |
include/clad/Differentiator/ArrayRef.h:241:
- std::is_same<T, std::nullptr_t>::value>::type>
+ std::is_same<T, std::nullptr_t>::value>>| array_ref() = delete; | ||
|
|
||
| template <typename T, class = typename std::enable_if< | ||
| std::is_pointer<T>::value || |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use c++17 style variable templates [modernize-type-traits]
| std::is_pointer<T>::value || | |
| std::is_pointer_v<T> || |
|
|
||
| template <typename T, class = typename std::enable_if< | ||
| std::is_pointer<T>::value || | ||
| std::is_same<T, std::nullptr_t>::value>::type> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: no header providing "std::nullptr_t" is directly included [misc-include-cleaner]
std::is_same<T, std::nullptr_t>::value>::type>
^|
|
||
| template <typename T, class = typename std::enable_if< | ||
| std::is_pointer<T>::value || | ||
| std::is_same<T, std::nullptr_t>::value>::type> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: use c++17 style variable templates [modernize-type-traits]
| std::is_same<T, std::nullptr_t>::value>::type> | |
| std::is_same_v<T, std::nullptr_t>>::type> |
| /// A specialization for C arrays | ||
| template <typename T, std::size_t N, std::size_t SBO_SIZE = 64, | ||
| std::size_t SLAB_SIZE = 1024> | ||
| CUDA_HOST_DEVICE void pop(tape<T[N], SBO_SIZE, SLAB_SIZE>& to) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
CUDA_HOST_DEVICE void pop(tape<T[N], SBO_SIZE, SLAB_SIZE>& to) {
^| /// A specialization for C arrays | ||
| template <typename T, typename U, size_t N, std::size_t SBO_SIZE = 64, | ||
| std::size_t SLAB_SIZE = 1024> | ||
| void push(tape<T[N], SBO_SIZE, SLAB_SIZE, /*is_multithreaded=*/true>& to, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
void push(tape<T[N], SBO_SIZE, SLAB_SIZE, /*is_multithreaded=*/true>& to,
^| /// A specialization for C arrays | ||
| template <typename T, std::size_t N, std::size_t SBO_SIZE = 64, | ||
| std::size_t SLAB_SIZE = 1024> | ||
| void pop(tape<T[N], SBO_SIZE, SLAB_SIZE, /*is_multithreaded=*/true>& to) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: do not declare C-style arrays, use std::array<> instead [cppcoreguidelines-avoid-c-arrays]
void pop(tape<T[N], SBO_SIZE, SLAB_SIZE, /*is_multithreaded=*/true>& to) {
^
The Problem
I noticed that building CLAD with
-std=c++11fails right now. The issue is thatArrayRef.handDifferentiator.hare using some "relaxed constexpr" features (like loops,ifstatements, and multiple returns) that are only allowed in C++14 and newer.The Fix
I added a simple compatibility macro called
CLAD_CONSTEXPR_CXX14.constexpr.inline. This keeps the code efficient but allows it to actually compile under the stricter C++11 rules.What I Changed
Differentiator.handArrayRef.h.constexprfor the macro in the crashing functions (mostly inArrayRef::operator=andslice).autoreturn type was missing its trailing return type (another C++11 requirement).Verification
I verified this locally by running
clang++ -std=c++11 ...against a demo file, and it builds cleanly now without the constexpr errors.