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

PPS: code to copy-without-children moved from copy constructor to a named method #31492

Merged
merged 2 commits into from Sep 22, 2020
Merged

PPS: code to copy-without-children moved from copy constructor to a named method #31492

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0e26c4a
Code to copy-without-children moved from copy constructor to a named
jan-kaspar Sep 17, 2020
4e4ac6d
Introduce copy constructor with copy options.
jan-kaspar Sep 18, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
6 changes: 4 additions & 2 deletions Geometry/VeryForwardGeometryBuilder/interface/DetGeomDesc.h
View file
Open in desktop
Expand Up @@ -53,15 +53,17 @@ class DetGeomDesc {
using RotationMatrix = ROOT::Math::Rotation3D;
using Translation = ROOT::Math::DisplacementVector3D<ROOT::Math::Cartesian3D<double>>;

DetGeomDesc() = default;
Copy link
Contributor

Choose a reason for hiding this comment

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

Why adding the default constructor?
A DetGeomDesc built with this, will not have interesting data members, and no way of setting them.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A way of creating an "empty" object is needed here:
https://github.com/cms-sw/cmssw/pull/31492/files/0e26c4a0f7658a37176349a8e446492503cdf81c#diff-cae30eec6c3de96aa4e668edf94cf7a8R62
Would you have a better suggestion?

Copy link
Contributor

Choose a reason for hiding this comment

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

Make it private?

Copy link
Contributor

@ghugo83 ghugo83 Sep 17, 2020 •
edited

Choose a reason for hiding this comment

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

I think there should just be a custom copy constructor.
This in a sense is just 'trying to have' a constructor, with settings done in makeCopyWithoutChildren()


// Constructor from old DD DDFilteredView
DetGeomDesc(const DDFilteredView& fv);
// Constructor from DD4Hep DDFilteredView
DetGeomDesc(const cms::DDFilteredView& fv);

DetGeomDesc(const DetGeomDesc&);
DetGeomDesc& operator=(const DetGeomDesc&);
virtual ~DetGeomDesc();

DetGeomDesc* makeCopyWithoutChildren() const;
Copy link
Contributor

@makortel makortel Sep 17, 2020 •
edited

Choose a reason for hiding this comment

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

Can this be made to return std::unique_ptr<DetGeomDesc> to make it explicit that the caller gets the ownership?

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like a return by value would work as well. If so, that is better than returning by pointer or unique_ptr.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The current code works with pointers:

cmssw/Geometry/VeryForwardGeometryBuilder/interface/DetGeomDesc.h

Line 52 in 0e26c4a

using Container = std::vector<DetGeomDesc*>;

cmssw/Geometry/VeryForwardGeometryBuilder/plugins/CTPPSGeometryESModule.cc

Line 131 in 0e26c4a

DetGeomDesc* alignedDetRoot = idealDetRoot.makeCopyWithoutChildren();

thus I proposed a function returning a pointer. Switching to unique_ptr is trivial. Let me know what your suggestion is.

Copy link
Contributor

Choose a reason for hiding this comment

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

One option would be to add a (private) constructor that takes all

cmssw/Geometry/VeryForwardGeometryBuilder/src/DetGeomDesc.cc

Lines 65 to 76 in 0e26c4a

output->m_name = m_name;
output->m_copy = m_copy;
output->m_isDD4hep = m_isDD4hep;
output->m_trans = m_trans;
output->m_rot = m_rot;
output->m_params = m_params;
output->m_isABox = m_isABox;
output->m_diamondBoxParams = m_diamondBoxParams;
output->m_sensorType = m_sensorType;
output->m_geographicalID = m_geographicalID;
output->m_z = m_z;

as parameters.

Copy link
Contributor

Choose a reason for hiding this comment

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

I was thinking it might be better to remove makCopyWithoutChildren and create a constructor that takes a DetGeomDesc const& and a 'dummy' enum with the only value noChildren. Then the calling code would become

DetGeomDesc* alignedDetRoot = new DetGeomDesc(idealDetRoot, DetGeomDesc::noChildren);

Copy link
Contributor

Choose a reason for hiding this comment

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

@Dr15Jones Yes exactly
+10000

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Many thanks for all the suggestions! I also like this one. I will give it a try tomorrow.


// general info
const std::string& name() const { return m_name; }
int copyno() const { return m_copy; }
Expand Down
4 changes: 2 additions & 2 deletions Geometry/VeryForwardGeometryBuilder/plugins/CTPPSGeometryESModule.cc
View file
Open in desktop
Expand Up @@ -128,7 +128,7 @@ std::unique_ptr<DetGeomDesc> CTPPSGeometryESModule::applyAlignments(const DetGeo
bufferIdealGeo.emplace_back(&idealDetRoot);

std::deque<DetGeomDesc*> bufferAlignedGeo;
DetGeomDesc* alignedDetRoot = new DetGeomDesc(idealDetRoot);
DetGeomDesc* alignedDetRoot = idealDetRoot.makeCopyWithoutChildren();
bufferAlignedGeo.emplace_back(alignedDetRoot);

while (!bufferIdealGeo.empty()) {
Expand Down Expand Up @@ -169,7 +169,7 @@ std::unique_ptr<DetGeomDesc> CTPPSGeometryESModule::applyAlignments(const DetGeo
bufferIdealGeo.emplace_back(idealDetChild);

// create new node with the same information as in idealDetChild and add it as a child of alignedDet
DetGeomDesc* alignedDetChild = new DetGeomDesc(*idealDetChild);
DetGeomDesc* alignedDetChild = idealDetChild->makeCopyWithoutChildren();
alignedDet->addComponent(alignedDetChild);

bufferAlignedGeo.emplace_back(alignedDetChild);
Expand Down
36 changes: 19 additions & 17 deletions Geometry/VeryForwardGeometryBuilder/src/DetGeomDesc.cc
View file
Open in desktop
Expand Up @@ -58,21 +58,24 @@ DetGeomDesc::DetGeomDesc(const cms::DDFilteredView& fv)
m_z(geant_units::operators::convertCmToMm(fv.translation().z())) // converted from cm (DD4hep) to mm
{}

DetGeomDesc::DetGeomDesc(const DetGeomDesc& ref) { (*this) = ref; }

DetGeomDesc& DetGeomDesc::operator=(const DetGeomDesc& ref) {
m_name = ref.m_name;
m_copy = ref.m_copy;
m_isDD4hep = ref.m_isDD4hep;
m_trans = ref.m_trans;
m_rot = ref.m_rot;
m_params = ref.m_params;
m_isABox = ref.m_isABox;
m_diamondBoxParams = ref.m_diamondBoxParams;
m_sensorType = ref.m_sensorType;
m_geographicalID = ref.m_geographicalID;
m_z = ref.m_z;
return (*this);
DetGeomDesc* DetGeomDesc::makeCopyWithoutChildren() const {
Copy link
Contributor

Choose a reason for hiding this comment

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

Yes the renaming is a good idea

DetGeomDesc* output = new DetGeomDesc;

// copy all data members except children (m_container)
output->m_name = m_name;
output->m_copy = m_copy;
output->m_isDD4hep = m_isDD4hep;
output->m_trans = m_trans;
output->m_rot = m_rot;
output->m_params = m_params;
output->m_isABox = m_isABox;
output->m_diamondBoxParams = m_diamondBoxParams;
output->m_sensorType = m_sensorType;
output->m_geographicalID = m_geographicalID;

output->m_z = m_z;

return output;
}

DetGeomDesc::~DetGeomDesc() { deepDeleteComponents(); }
Expand Down Expand Up @@ -116,8 +119,7 @@ void DetGeomDesc::deleteComponents() { m_container.erase(m_container.begin(), m_

void DetGeomDesc::deepDeleteComponents() {
for (auto& it : m_container) {
it->deepDeleteComponents();
delete it;
delete it; // the destructor calls deepDeleteComponents
}
clearComponents();
}
Expand Down