Skip to content
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

Modernize to c++14 for gcc/clang like compilers #1074

Merged
merged 74 commits into from Dec 10, 2023

Conversation

marscher
Copy link
Member

@marscher marscher commented May 29, 2022

So far, we have used c++14 standard only for the Windows platform, but Unix still used c++11.
This PR modernizes some constructs already introduced in c++11, but really not used so far. I've used clang-tidy to provide most of the fixes.

  • Use "explicit" keyword for constructors to avoid implicitly casting something unrelated.
  • Virtual constructors are now marked "override". Virtual and override together makes no sense (according to clang-tidy).
  • prefer "using type_alias = type_x" semantics over "typedef"
  • Use range operator to simplify statements
  • Use "auto" where applicable
  • use "nullptr" over macro NULL
  • use variable initializers (avoids code duplication)
  • use default ctors/dtors to shrink code and allow for further compiler optimization
  • use std::move semantics (string moves apparently)
  • mark unimplemented private special functions with "delete" and made them public
  • fixed: uninitialized member vars for some classes (JPContext etc.)

In addition I cleaned up JPype.h to only contain STL related stuff, if it is mandatory to declare types used within headers. The implementation now includes what is really needed. This should speed up compilation a little bit.

Another important change is that JPypeException now derives from std::runtime_error, which adds some additional safety. The copy constructor of JPypeException was not marked "noexcept", so it could have thrown exceptions during the initialization (leading to a terminate call). As I didn't get the logic why the (string) message gets copied into a stringstream without any other additions just to make a string and then a cstring again, I just got rid of this behaviour.
By using a std exception type, we are safe that we use the proper standard definitions for exception safety. If this is undesired, I can also revert these changes.

@marscher
Copy link
Member Author

marscher commented Jun 1, 2022

I belief that it was another situation back then. There was a plethora of different available compilers, which nowadays have converged to LLVM/CLang, GCC and MSVC basically. In the past we have accepted patches for all kinds of exotic combinations (e.g. Gcc on Cygwin).
But I trust your experience and revert the change of C++ standard back to C++11 for all GCC/CLang based compilers. MSVC uses C++14. This is the same behaviour as for the released versions.

@Thrameos
Copy link
Contributor

Thrameos commented Jun 1, 2022

We are probably safe to go all C++14 if that would be easier to deal with. I just won't go to C++20 for a long while.

@Thrameos
Copy link
Contributor

Thrameos commented Jun 1, 2022

I see codecov is mad about a bunch of missing test cases. I guess I will take another shot at improving coverage after this one goes in.

@marscher marscher marked this pull request as ready for review June 3, 2022 07:20
@Thrameos
Copy link
Contributor

Thrameos commented Apr 4, 2023

I am hoping to get out 1.5 this month. Is this PR in good shape?

@Thrameos
Copy link
Contributor

@marscher should I try to get this included in 1.5

@marscher
Copy link
Member Author

Sorry for the delay. I guess this is in a good shape - it has been a while since I looked into it. One major change is that JPypeExceptions are now children of std::runtime_error. Honestly I cannot tell if this has any unwanted side effects.

return data[0];
}
if (_PyUnicode_STATE(self).kind == PyUnicode_1BYTE_KIND)
{
Py_UCS1 *data = (Py_UCS1*) ( ((PyCompactUnicodeObject*) self) + 1);
auto *data = (Py_UCS1*) ( ((PyCompactUnicodeObject*) self) + 1);

Check failure

Code scanning / CodeQL

Suspicious pointer scaling High

This pointer might have type
PyJPChar
(size 80), but this pointer arithmetic is done with type PyCompactUnicodeObject * (size 72).
return data[0];
}
Py_UCS2 *data = (Py_UCS2*) ( ((PyCompactUnicodeObject*) self) + 1);
auto *data = (Py_UCS2*) ( ((PyCompactUnicodeObject*) self) + 1);

Check failure

Code scanning / CodeQL

Suspicious pointer scaling High

This pointer might have type
PyJPChar
(size 80), but this pointer arithmetic is done with type PyCompactUnicodeObject * (size 72).
self->m_Obj.utf8_length = 0;
} else
{

Py_UCS2 *data = (Py_UCS2*) ( ((PyCompactUnicodeObject*) self) + 1);
auto *data = (Py_UCS2*) ( ((PyCompactUnicodeObject*) self) + 1);

Check failure

Code scanning / CodeQL

Suspicious pointer scaling High

This pointer might have type
_object
(size 16), but this pointer arithmetic is done with type PyCompactUnicodeObject * (size 72).
native/common/jp_exception.cpp Dismissed Show dismissed Hide dismissed
m_Error.l = errType;
m_Message = msn;
//m_Message = msn;

Check notice

Code scanning / CodeQL

Commented-out code Note

This comment appears to contain commented-out code.
@@ -154,15 +154,15 @@
{
if (_PyUnicode_STATE(self).ascii == 1)
{
Py_UCS1 *data = (Py_UCS1*) (((PyASCIIObject*) self) + 1);
auto *data = (Py_UCS1*) (((PyASCIIObject*) self) + 1);

Check failure

Code scanning / CodeQL

Suspicious pointer scaling High

This pointer might have type
PyJPChar
(size 80), but this pointer arithmetic is done with type PyASCIIObject * (size 48).
jpype/_core.py Dismissed Show dismissed Hide dismissed
@marscher marscher added this to the 1.5 milestone Apr 29, 2023
@Thrameos Thrameos merged commit 9d53015 into master Dec 10, 2023
5 of 22 checks passed
@marscher marscher deleted the tidyup_virtual_override_mess branch April 9, 2024 19:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants