jpype

jpype copied to clipboard

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.

May 29 '22 15:05 marscher

Codecov Report

Attention: 268 lines in your changes are missing coverage. Please review.

Comparison is base (33597f8) 87.83% compared to head (5647a3a) 87.31%. Report is 9 commits behind head on master.

:exclamation: Current head 5647a3a differs from pull request most recent head f06042f. Consider uploading reports for the commit f06042f to get more accurate results

@@            Coverage Diff             @@
##           master    #1074      +/-   ##
==========================================
- Coverage   87.83%   87.31%   -0.52%     
==========================================
  Files         112      113       +1     
  Lines       10283    10209      -74     
  Branches     4034     4049      +15     
==========================================
- Hits         9032     8914     -118     
- Misses        699      702       +3     
- Partials      552      593      +41     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

May 29 '22 15:05 codecov[bot]

This pull request introduces 1 alert when merging 78819bb7ebefbb948ac3bfee067bc4fd8e86ffcf into f38a0665997c01fb70b25495eebaae361b268cc4 - view on LGTM.com

new alerts:

• 1 for Inconsistent definition of copy constructor and assignment ('Rule of Two')

May 29 '22 17:05 lgtm-com[bot]

Looks really extensive! Since there is such significant churn as a result, does it make sense to apply a formatter (e.g. clang-format) to help with code consistency? (I don't propose it in this PR, but it could be a reasonable follow-on PR).

May 30 '22 08:05 pelson

I would ask that we do not run a formatter. It would kill epypj branch which is a huge work efforr that just needed people to help write tests.

May 30 '22 12:05 Thrameos

@marscher thanks for tackling this one. Unfortunately my knowledge of C++ is circa 2000 and there were still a lot of very bad compilers at the time so some of my coding style is to avoid problems in ancient compilers. Things like letting compiler do default things rather than implement the in source file resulted in blotted binaries with mulple vtable copies and other horrible.

Looking over most looks reasonable. Though the curl brace initializers look really odd to me. I am guessing the weird path with exections was so that I could add source line numbers to the exception which was removed when I added the stack trace linkage.

May 30 '22 12:05 Thrameos

Thanks for taking a look guys! The question is, if we want to support ancient compilers in an age where c++20(20!) is about to be released. I have to be honest, and admit, that I have to update my c++ knowledge as well. The curly brace initializer just invoke the default constructor, e.g. a raw-pointer (which we actually should stop using) it'd be initialized with a nullptr.

I'm also not favouring an automatic code formatting (right now), as it would break (in a merge-things-back manner) development processes.

May 31 '22 09:05 marscher

So is this ready for a review at this point or is it still a work in progress?

(Actually I am confused.... the top of the page says "review requested" and the bottom says "work in progress", so I just need to scroll down.)

May 31 '22 18:05 Thrameos

My general opinion on new compilers like C++20 when supporting a library which is supposed to work from Python 3.7 to current, is that we should stick with features that were considered to be well established when the oldest binary was released. "Well established" is of course hard to define. When I was doing gtkmm in late 1990s, I would have to find the common features from Sun, HPUX, gcc, visual studio and a hand full of others. There were plenty of useful features in C++ that would make life easier, but inevitably one of the compilers would have a bug of some kind (and that meant finding one of those machines in a lab somewhere and trying to debug the problem there). Thus I ended deliberately avoiding anything that was near the edge (even when it made for cruddy looking code).

I still think that we should keep with the same philosophy, though as I don't do as heavy C++ development, I will have to defer to others as to what is well established and what is marginal. While doing the threading tests, I used C11 features only to discover they don't exist on Macos and that was stuff adopted over 10 years ago.

So long story short, don't go hog wild using every new language feature.

Jun 01 '22 15:06 Thrameos

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.

Jun 01 '22 17:06 marscher

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.

Jun 01 '22 20:06 Thrameos

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.

Jun 01 '22 20:06 Thrameos

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

Apr 04 '23 00:04 Thrameos

@marscher should I try to get this included in 1.5

Apr 20 '23 19:04 Thrameos

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.

Apr 25 '23 06:04 marscher