-
Notifications
You must be signed in to change notification settings - Fork 278
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
Upgrade C++ standard to c++17 #2052
Changes from 2 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -25,6 +25,10 @@ THE SOFTWARE. | |
#define JzonAPI __declspec(dllexport) | ||
#endif | ||
|
||
#ifdef WIN32 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This should be _MSC_VER or something. Not relevant for mingw. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Speaking of this it would probably be a good idea to set up an msys CI There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The CI does build MinGW/msys2. We don't have that in the nightly GHA build. Would you like to change/fix that? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We actually have that MSYS2 build which is triggered on PRs: I think it is enough to have it there and not in the nightly builds. Now that I think about it, on Windows + MSYS2, it is ending up using MinGW (with gcc-11). The problem with the |
||
#define _SILENCE_ALL_CXX17_DEPRECATION_WARNINGS | ||
#endif | ||
|
||
#include "Jzon.h" | ||
|
||
#include <algorithm> | ||
|
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.
AFAIK inline here is redundant. static implies inline. This should also be constexpr.
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.
Maybe even std::string_view. Although I assume that would break API.
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.
I do not think
static
impliesinline
. Theinline
specified for static variable members is something new in C++17:https://en.cppreference.com/w/cpp/language/inline
I just did this change because the previous code was failing to compile with C++17.
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.
it's constexpr that implies inline.
what compile error?
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.
I just checked it out. It was actually not a compiler error but linking one:
I think the problem is that in the header file it was declared as:
While in the cpp file:
I could also declare it as
constexpr
in the header file as you suggested so that we do not need to add theinline
specifier.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.
No that error has to do with Windows and dllexport. defining them in the header is the correct solution.
Small nitpick: static constexpr vs. constexpr static.
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.
I actually got the error on Ubuntu 20.4 with Gcc-10 😅 . I'll change the order to
static constexpr
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.
I just realized. C++17 supports CTAD. You can safely ommit the template parameters.