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

Hide IsoSpec using pimpl and add some clang-tidy performance fixes on top #5681

Merged
merged 6 commits into from
Dec 2, 2021

Conversation

timosachsenberg
Copy link
Contributor

@timosachsenberg timosachsenberg commented Nov 24, 2021

Description

clang-tidy fixes
cppcoreguidelines-virtual-class-destructor
misc-redundant-expression,
performance-trivially-destructible
performance-noexcept-move-constructor

Checklist:

  • Make sure that you are listed in the AUTHORS file
  • Add relevant changes and new features to the CHANGELOG file
  • I have commented my code, particularly in hard-to-understand areas
  • New and existing unit tests pass locally with my changes
  • Updated or added python bindings for changed or new classes. (Tick if no updates were necessary.)

How can I get additional information on failed tests during CI:

If your PR is failing you can check out

Note:

  • Once you opened a PR try to minimize the number of pushes to it as every push will trigger CI (automated builds and test) and is rather heavy on our infrastructure (e.g., if several pushes per day are performed).

…performance-trivially-destructible,performance-noexcept-move-constructor
@hroest
Copy link
Contributor

hroest commented Nov 25, 2021

I see this is helpful, but how does clang know that something cannot throw?

@timosachsenberg timosachsenberg changed the title clang-tidy performance 2 Hide IsoSpec using pimpl and add some clang-tidy performance fixes on top Nov 28, 2021
@@ -214,6 +239,8 @@ namespace OpenMS
return ID;
}

IsoSpecThresholdWrapper::~IsoSpecThresholdWrapper() = default;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Today I learned that in case of these partial types we also need to put the default destructor into the cpp file, In header it fails.

@timosachsenberg
Copy link
Contributor Author

tests passed


protected:
IsoSpec::IsoLayeredGenerator ILG;
IsoSpecTotalProbGeneratorWrapper(const IsoSpecTotalProbGeneratorWrapper&) = delete;
Copy link
Contributor

Choose a reason for hiding this comment

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

nitpicking here but:
would it make sense to move the copy ctor up into the area of all other ctors?! Just to have them all in one place, in case someone is checking them and sees that this one seems "missing"?



protected:
IsoSpec::IsoThresholdGenerator ITG;
IsoSpecThresholdGeneratorWrapper(const IsoSpecThresholdGeneratorWrapper&) = delete;
Copy link
Contributor

Choose a reason for hiding this comment

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

same here

@cbielow
Copy link
Contributor

cbielow commented Nov 29, 2021

I see this is helpful, but how does clang know that something cannot throw?

Do you mean noexcept? It cannot. We are just promising it does not (or bare the consequences).

@cbielow
Copy link
Contributor

cbielow commented Nov 29, 2021

looks fine to me

@@ -45,17 +46,21 @@
#include <OpenMS/CHEMISTRY/EmpiricalFormula.h>
#include <OpenMS/CHEMISTRY/ISOTOPEDISTRIBUTION/IsotopeDistribution.h>


// Override IsoSpec's use of mmap whenever it is available
#define ISOSPEC_GOT_SYSTEM_MMAN false
#define ISOSPEC_GOT_MMAN false
#define ISOSPEC_BUILDING_OPENMS true
Copy link
Contributor

Choose a reason for hiding this comment

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

I assume you then also dont need these definitions in the header any more?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes I will remove them

@timosachsenberg
Copy link
Contributor Author

rebuild jenkins

@timosachsenberg
Copy link
Contributor Author

passed

@timosachsenberg timosachsenberg merged commit 48f80b3 into develop Dec 2, 2021
@timosachsenberg timosachsenberg deleted the clang-tidy_performance2 branch December 2, 2021 11:48
@hroest
Copy link
Contributor

hroest commented Dec 3, 2021

I see this is helpful, but how does clang know that something cannot throw?

Do you mean noexcept? It cannot. We are just promising it does not (or bare the consequences).

@cbielow @timosachsenberg yes exactly that was my point, how can clang-tidy automatically add noexcept -- shouldnt we check this manually?

@timosachsenberg
Copy link
Contributor Author

I think it checks all called functions for throw. If there is none it adds noexcept.

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