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.

Sign up for GitHub

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

Jump to bottom

[RECONSTRUCTION] code-checks/format; clean code to remove global functions #34140

Merged
merged 10 commits into from
Jun 23, 2021
Merged

[RECONSTRUCTION] code-checks/format; clean code to remove global functions #34140

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
5a3b474
[RECONSTRUCTION] Apply code-checks with misc-definitions-in-headers
smuzaffar Jun 15, 2021
f38cace
[RECONSTRUCTION] code format
smuzaffar Jun 15, 2021
f624155
clang tidy/format
smuzaffar Jun 16, 2021
d3f1048
cleanup global function by either moving them to class or merge the .…
smuzaffar Jun 21, 2021
001ef12
no need to have inline keyword in class body
smuzaffar Jun 21, 2021
5400188
use static instead of const methods
smuzaffar Jun 21, 2021
240f5d7
optimized format_vstring to not relocate retVal N time
smuzaffar Jun 22, 2021
0f62bae
code-format
smuzaffar Jun 22, 2021
6935f28
remove dead code in Conv4HitsReco.cc
smuzaffar Jun 22, 2021
3eb8cb5
move KFFittingSmoother.h in to its source file
smuzaffar Jun 22, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 7 additions & 6 deletions CommonTools/RecoAlgos/plugins/PrimaryVertexSorter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -301,27 +301,28 @@ void PrimaryVertexSorter<ParticlesCollection>::produce(edm::Event& iEvent, const
}

template <>
void PrimaryVertexSorter<std::vector<reco::RecoChargedRefCandidate>>::doConsumesForTiming(
inline void PrimaryVertexSorter<std::vector<reco::RecoChargedRefCandidate>>::doConsumesForTiming(
const edm::ParameterSet& iConfig) {
tokenTrackTimeTag_ = consumes<edm::ValueMap<float>>(iConfig.getParameter<edm::InputTag>("trackTimeTag"));
tokenTrackTimeResoTag_ = consumes<edm::ValueMap<float>>(iConfig.getParameter<edm::InputTag>("trackTimeResoTag"));
}

template <>
void PrimaryVertexSorter<std::vector<reco::PFCandidate>>::doConsumesForTiming(const edm::ParameterSet& iConfig) {}
inline void PrimaryVertexSorter<std::vector<reco::PFCandidate>>::doConsumesForTiming(const edm::ParameterSet& iConfig) {
}

template <>
bool PrimaryVertexSorter<std::vector<reco::RecoChargedRefCandidate>>::needsProductsForTiming() {
inline bool PrimaryVertexSorter<std::vector<reco::RecoChargedRefCandidate>>::needsProductsForTiming() {
return true;
}

template <>
bool PrimaryVertexSorter<std::vector<reco::PFCandidate>>::needsProductsForTiming() {
inline bool PrimaryVertexSorter<std::vector<reco::PFCandidate>>::needsProductsForTiming() {
return false;
}

template <>
std::pair<int, PrimaryVertexAssignment::Quality>
inline std::pair<int, PrimaryVertexAssignment::Quality>
PrimaryVertexSorter<std::vector<reco::RecoChargedRefCandidate>>::runAlgo(const reco::VertexCollection& vertices,
const reco::RecoChargedRefCandidate& pf,
const edm::ValueMap<float>* trackTimeTag,
Expand All @@ -332,7 +333,7 @@ PrimaryVertexSorter<std::vector<reco::RecoChargedRefCandidate>>::runAlgo(const r
}

template <>
std::pair<int, PrimaryVertexAssignment::Quality> PrimaryVertexSorter<std::vector<reco::PFCandidate>>::runAlgo(
inline std::pair<int, PrimaryVertexAssignment::Quality> PrimaryVertexSorter<std::vector<reco::PFCandidate>>::runAlgo(
const reco::VertexCollection& vertices,
const reco::PFCandidate& pf,
const edm::ValueMap<float>* trackTimeTag,
Expand Down
29 changes: 15 additions & 14 deletions DataFormats/DTRecHit/interface/DTDriftTimeParameters.icc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -404,7 +404,8 @@ const double THIS_CLASS::fun_sigma_t[N_alpha][N_By][N_Bz][N_Sigma_t] = {

/*** Auxiliary functions ***/

unsigned short THIS_CLASS::MB_DT_Check_boundaries(double distime, double alpha, double by, double bz, short ifl) const {
inline unsigned short THIS_CLASS::MB_DT_Check_boundaries(
double distime, double alpha, double by, double bz, short ifl) const {
unsigned short status = 1;
std::string name = "MB_DT_drift_time";

Expand Down Expand Up @@ -458,7 +459,7 @@ unsigned short THIS_CLASS::MB_DT_Check_boundaries(double distime, double alpha,
return (status);
}

void THIS_CLASS::MB_DT_Get_grid_values(
inline void THIS_CLASS::MB_DT_Get_grid_values(
double Var, unsigned short *pi, unsigned short *pj, short Initial, unsigned short N, const double *Values) const {
unsigned short i, iValue, jValue;

Expand All @@ -482,15 +483,15 @@ void THIS_CLASS::MB_DT_Get_grid_values(
}
}

void THIS_CLASS::MB_DT_Get_grid_points(double alpha,
double by,
double bz,
unsigned short *p_alpha,
unsigned short *p_By,
unsigned short *p_Bz,
unsigned short *q_alpha,
unsigned short *q_By,
unsigned short *q_Bz) const {
inline void THIS_CLASS::MB_DT_Get_grid_points(double alpha,
double by,
double bz,
unsigned short *p_alpha,
unsigned short *p_By,
unsigned short *p_Bz,
unsigned short *q_alpha,
unsigned short *q_By,
unsigned short *q_Bz) const {
MB_DT_Get_grid_values(fabs(alpha), p_alpha, q_alpha, 4, N_alpha, alpha_value);
MB_DT_Get_grid_values(by, p_By, q_By, 0, N_By, By_value);
MB_DT_Get_grid_values(bz, p_Bz, q_Bz, 0, N_Bz, Bz_value);
Expand All @@ -503,7 +504,7 @@ void THIS_CLASS::MB_DT_Get_grid_points(double alpha,

/*** Multidimensional linear interpolation ***/

double THIS_CLASS::MB_DT_MLInterpolation(double *al, double *by, double *bz, double *f) const {
inline double THIS_CLASS::MB_DT_MLInterpolation(double *al, double *by, double *bz, double *f) const {
double q1, q2, q3, p1, p2, p3;
double fx11, fx21, fxy1, fx12, fx22, fxy2, fxyz;

Expand All @@ -528,7 +529,7 @@ double THIS_CLASS::MB_DT_MLInterpolation(double *al, double *by, double *bz, dou
return (fxyz);
}

double THIS_CLASS::MB_DT_sigma_t_m(double dist, double *par) const {
inline double THIS_CLASS::MB_DT_sigma_t_m(double dist, double *par) const {
double x = fabs(dist); // the parametrisations are symmetric under 'distance'

if (x > 20.5)
Expand All @@ -537,7 +538,7 @@ double THIS_CLASS::MB_DT_sigma_t_m(double dist, double *par) const {
return (par[6] * x);
}

double THIS_CLASS::MB_DT_sigma_t_p(double dist, double *par) const {
inline double THIS_CLASS::MB_DT_sigma_t_p(double dist, double *par) const {
double x2, x = fabs(dist); // the parametrisations are symmetric under 'distance'

if (x > 20.5)
Expand Down
8 changes: 4 additions & 4 deletions EventFilter/HcalRawToDigi/plugins/PackerHelp.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -617,10 +617,10 @@ class UHTRpacker {

// converts HE QIE digies to HB data format

QIE11DataFrame convertHB(QIE11DataFrame qiehe,
std::vector<int> const& tdc1,
std::vector<int> const& tdc2,
const int tdcmax) {
inline QIE11DataFrame convertHB(QIE11DataFrame qiehe,
std::vector<int> const& tdc1,
std::vector<int> const& tdc2,
const int tdcmax) {
QIE11DataFrame qiehb = qiehe;
HcalDetId did = HcalDetId(qiehb.detid());
int adc, tdc;
Expand Down
2 changes: 1 addition & 1 deletion MagneticField/GeomBuilder/src/buildBox.icc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,7 @@
* \author N. Amapane - INFN Torino
*/

void volumeHandle::buildBox(double halfX, double halfY, double halfZ) {
inline void volumeHandle::buildBox(double halfX, double halfY, double halfZ) {
LogTrace("MagGeoBuilder") << "Building box surfaces...: ";

// Global vectors of the normals to X, Y, Z axes
Expand Down
14 changes: 7 additions & 7 deletions MagneticField/GeomBuilder/src/buildCons.icc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,13 +6,13 @@

#include "DataFormats/GeometrySurface/interface/SimpleConeBounds.h"

void volumeHandle::buildCons(double zhalf,
double rInMinusZ,
double rOutMinusZ,
double rInPlusZ,
double rOutPlusZ,
double startPhi,
double deltaPhi) {
inline void volumeHandle::buildCons(double zhalf,
double rInMinusZ,
double rOutMinusZ,
double rInPlusZ,
double rOutPlusZ,
double startPhi,
double deltaPhi) {
LogTrace("MagGeoBuilder") << "Building cons surfaces...: ";
LogTrace("MagGeoBuilder") << "zhalf " << zhalf << newln << "rInMinusZ " << rInMinusZ << newln << "rOutMinusZ "
<< rOutMinusZ << newln << "rInPlusZ " << rInPlusZ << newln << "rOutPlusZ " << rOutPlusZ
Expand Down
2 changes: 1 addition & 1 deletion MagneticField/GeomBuilder/src/buildPseudoTrap.icc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,7 @@
* \author N. Amapane - INFN Torino
*/

void volumeHandle::buildPseudoTrap(
inline void volumeHandle::buildPseudoTrap(
double x1, double x2, double y1, double y2, double halfZ, double radius, bool atMinusZ) {
LogTrace("MagGeoBuilder") << "Building PseudoTrap surfaces...: ";
LogTrace("MagGeoBuilder") << "halfZ " << halfZ << newln << "x1 " << x1 << newln << "x2 " << x2
Expand Down
22 changes: 11 additions & 11 deletions MagneticField/GeomBuilder/src/buildTrap.icc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,17 @@
* \author N. Amapane - INFN Torino
*/

void volumeHandle::buildTrap(double x1,
double x2,
double x3,
double x4,
double y1,
double y2,
double theta,
double phi,
double halfZ,
double alpha1,
double alpha2) {
inline void volumeHandle::buildTrap(double x1,
double x2,
double x3,
double x4,
double y1,
double y2,
double theta,
double phi,
double halfZ,
double alpha1,
double alpha2) {
LogTrace("MagGeoBuilder") << "Building trapezoid surfaces...: ";
LogTrace("MagGeoBuilder") << "x1 " << x1 << newln << "x2 " << x2 << newln << "x3 " << x3 << newln << "x4 " << x4
<< newln << "y1 " << y1 << newln << "y2 " << y2 << newln << "theta " << theta << newln
Expand Down
16 changes: 8 additions & 8 deletions MagneticField/GeomBuilder/src/buildTruncTubs.icc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,14 +9,14 @@
* \author N. Amapane - INFN Torino
*/

void volumeHandle::buildTruncTubs(double zhalf,
double rIn,
double rOut,
double startPhi,
double deltaPhi,
double cutAtStart,
double cutAtDelta,
bool cutInside) {
inline void volumeHandle::buildTruncTubs(double zhalf,
double rIn,
double rOut,
double startPhi,
double deltaPhi,
double cutAtStart,
double cutAtDelta,
bool cutInside) {
LogTrace("MagGeoBuilder") << "Building TruncTubs surfaces...: ";
LogTrace("MagGeoBuilder") << "zhalf " << zhalf << newln << "rIn " << rIn << newln << "rOut " << rOut
<< newln << "startPhi " << startPhi << newln << "deltaPhi " << deltaPhi << newln
Expand Down
2 changes: 1 addition & 1 deletion MagneticField/GeomBuilder/src/buildTubs.icc
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
void volumeHandle::buildTubs(double zhalf, double rIn, double rOut, double startPhi, double deltaPhi) {
inline void volumeHandle::buildTubs(double zhalf, double rIn, double rOut, double startPhi, double deltaPhi) {
LogTrace("MagGeoBuilder") << "Building tubs surfaces...: ";
LogTrace("MagGeoBuilder") << "zhalf " << zhalf << newln << "rIn " << rIn << newln << "rOut " << rOut
<< newln << "startPhi " << startPhi << newln << "deltaPhi " << deltaPhi;
Expand Down
37 changes: 18 additions & 19 deletions PhysicsTools/PatUtils/interface/PATJetCorrExtractor.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,25 +29,6 @@
#include <string>
#include <vector>

namespace {
std::string format_vstring(const std::vector<std::string>& v) {
std::string retVal;

retVal.append("{ ");

unsigned numEntries = v.size();
for (unsigned iEntry = 0; iEntry < numEntries; ++iEntry) {
retVal.append(v[iEntry]);
if (iEntry < (numEntries - 1))
retVal.append(", ");
}

retVal.append(" }");

return retVal;
}
} // namespace

class PATJetCorrExtractor {
public:
reco::Candidate::LorentzVector operator()(
Expand Down Expand Up @@ -88,6 +69,24 @@ class PATJetCorrExtractor {

return corrJetP4;
}

private:
static std::string format_vstring(const std::vector<std::string>& v) {
Copy link
Contributor

@VinInn VinInn Jun 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

This code is schoolbook example of memory-churn! (how many times retVal is relocated?)
One may try this:

static std::string format_vstring(const std::vector<std::string>& v)  {
    std::string retVal;
    auto ss = std::accumulate(v.begin(),v.end(),2,[](int a,std::string const & s){return a+s.length()+2;});
    retVal.reserve(ss);
    for_each(v.begin(),v.end(),[&](std::string const & s){retVal += (retVal.empty() ? "{ " : ", ")+s;});
    retVal+= " }";
    return retVal;
  }

Copy link
Contributor Author

@smuzaffar smuzaffar Jun 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

How about using fmt::format e.g. return "{ "+ fmt::format("{}",fmt::join(v,", ")) + " }"; should replace this whole function

Copy link
Contributor

Choose a reason for hiding this comment

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

fmt is yet another library. When we move to c++20 we can use std::format
(btw when we move to c++20?)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ok updated as suugested in #34140 (comment) .
Many of c++20 features are already available in GCC 10, I guess once we have gcc10 as production arch then we can enable c++20

Copy link
Contributor

Choose a reason for hiding this comment

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

When we move to c++20 we can use std::format

No we can't, unfortunately, because currently only MSVC implements it https://en.cppreference.com/w/cpp/compiler_support.

std::string retVal;

retVal.append("{ ");

unsigned numEntries = v.size();
for (unsigned iEntry = 0; iEntry < numEntries; ++iEntry) {
retVal.append(v[iEntry]);
if (iEntry < (numEntries - 1))
retVal.append(", ");
}

retVal.append(" }");

return retVal;
}
};

#endif
2 changes: 1 addition & 1 deletion RecoBTag/FeatureTools/interface/SeedingTracksConverter.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ namespace btagbtvdeep {
bool computeProbabilities,
std::vector<btagbtvdeep::SeedingTrackFeatures>& seedingT_features_vector);

float logWithOffset(float v, float logOffset = 0) {
inline float logWithOffset(float v, float logOffset = 0) {
if (v == 0.)
return 0.;
return logOffset + log(std::fabs(v)) * std::copysign(1.f, v);
Expand Down
5 changes: 3 additions & 2 deletions RecoBTag/ImpactParameter/plugins/IPProducer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -441,7 +441,7 @@ void IPProducer<Container, Base, Helper>::checkEventSetup(const edm::EventSetup&
// Specialized templates used to fill 'descriptions'
// ------------ method fills 'descriptions' with the allowed parameters for the module ------------
template <>
void IPProducer<reco::TrackRefVector, reco::JTATagInfo, IPProducerHelpers::FromJTA>::fillDescriptions(
inline void IPProducer<reco::TrackRefVector, reco::JTATagInfo, IPProducerHelpers::FromJTA>::fillDescriptions(
edm::ConfigurationDescriptions& descriptions) {
edm::ParameterSetDescription desc;
desc.add<double>("maximumTransverseImpactParameter", 0.2);
Expand All @@ -462,7 +462,8 @@ void IPProducer<reco::TrackRefVector, reco::JTATagInfo, IPProducerHelpers::FromJ
}

template <>
void IPProducer<std::vector<reco::CandidatePtr>, reco::JetTagInfo, IPProducerHelpers::FromJetAndCands>::fillDescriptions(
inline void
IPProducer<std::vector<reco::CandidatePtr>, reco::JetTagInfo, IPProducerHelpers::FromJetAndCands>::fillDescriptions(
edm::ConfigurationDescriptions& descriptions) {
edm::ParameterSetDescription desc;
desc.add<double>("maximumTransverseImpactParameter", 0.2);
Expand Down
4 changes: 3 additions & 1 deletion RecoBTag/SecondaryVertex/interface/CombinedSVSoftLeptonComputer.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,9 @@ class CombinedSVSoftLeptonComputer : public CombinedSVComputer {
bool SoftLeptonFlip;
};

double CombinedSVSoftLeptonComputer::flipSoftLeptonValue(double value) const { return SoftLeptonFlip ? -value : value; }
inline double CombinedSVSoftLeptonComputer::flipSoftLeptonValue(double value) const {
return SoftLeptonFlip ? -value : value;
}

template <class IPTI, class SVTI>
reco::TaggingVariableList CombinedSVSoftLeptonComputer::operator()(const IPTI &ipInfo,
Expand Down
4 changes: 2 additions & 2 deletions RecoEgamma/EgammaTools/interface/EGExtraInfoModifierFromValueMaps.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -281,7 +281,7 @@ void EGXtraModFromVMObjFiller<int>::addValueToObject(

template <>
template <>
void EGXtraModFromVMObjFiller<egmodifier::EGID>::addValuesToObject(
inline void EGXtraModFromVMObjFiller<egmodifier::EGID>::addValuesToObject(
pat::Electron& obj,
const edm::Ptr<reco::Candidate>& ptr,
const std::unordered_map<std::string, edm::EDGetTokenT<edm::ValueMap<float>>>& vmaps_token,
Expand All @@ -299,7 +299,7 @@ void EGXtraModFromVMObjFiller<egmodifier::EGID>::addValuesToObject(

template <>
template <>
void EGXtraModFromVMObjFiller<egmodifier::EGID>::addValuesToObject(
inline void EGXtraModFromVMObjFiller<egmodifier::EGID>::addValuesToObject(
pat::Photon& obj,
const edm::Ptr<reco::Candidate>& ptr,
const std::unordered_map<std::string, edm::EDGetTokenT<edm::ValueMap<float>>>& vmaps_token,
Expand Down
Loading