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

Geometry/GEMGeometryBuilder/src/GEMGeometryBuilderFromDDD.cc const-cast fix #25913

Merged
merged 3 commits into from Feb 19, 2019
Merged

Geometry/GEMGeometryBuilder/src/GEMGeometryBuilderFromDDD.cc const-cast fix #25913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0a76102
Store constructed superchambers in a vector to avoid const_cast
watson-ij Feb 12, 2019
dc8d1ff
Delete overlapping identifier
watson-ij Feb 12, 2019
23ab7bd
Add superchamber after adding chambers, to avoid having to change the…
watson-ij Feb 13, 2019
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
2 changes: 1 addition & 1 deletion Geometry/GEMGeometry/interface/GEMGeometry.h
View file
Open in desktop
Expand Up @@ -119,7 +119,7 @@ class GEMGeometry : public TrackingGeometry {
mapIdToDet theMap;

std::vector<const GEMEtaPartition*> allEtaPartitions; // Are not owned by this class; are owned by their chamber.
std::vector<const GEMChamber*> allChambers; // Are not owned by this class; are owned by their chamber.
std::vector<const GEMChamber*> allChambers; // Are not owned by this class; are owned by their superchamber.
std::vector<const GEMSuperChamber*> allSuperChambers; // Are owned by this class.
std::vector<const GEMRing*> allRings; // Are owned by this class.
std::vector<const GEMStation*> allStations; // Are owned by this class.
Expand Down
10 changes: 5 additions & 5 deletions Geometry/GEMGeometryBuilder/src/GEMGeometryBuilderFromDDD.cc
View file
Open in desktop
Expand Up @@ -49,6 +49,7 @@ GEMGeometryBuilderFromDDD::build( GEMGeometry& theGeometry,
bool doSuper = fv.firstChild();
LogDebug("GEMGeometryBuilderFromDDD") << "doSuperChamber = " << doSuper;
// loop over superchambers
std::vector<GEMSuperChamber*> superChambers;
while (doSuper){

// getting chamber id from eta partitions
Expand All @@ -65,7 +66,7 @@ GEMGeometryBuilderFromDDD::build( GEMGeometry& theGeometry,
// making superchamber out of the first chamber layer including the gap between chambers
if (detIdCh.layer() == 1){// only make superChambers when doing layer 1
GEMSuperChamber *gemSuperChamber = buildSuperChamber(fv, detIdCh);
theGeometry.add(gemSuperChamber);
superChambers.push_back(gemSuperChamber);
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a big dodgy to keep a list of writeable pointers after adding the super chambers to theGeometry. This technique works now, but future developers of the GEMGeometry class might assume that the GemSuperChamber objects stored in GEMGeometry cannot change since there is no interface that allows modification.
Perhaps the super chambers shouldn't be added to the GEMGeometry until after they are complete, but such an approach might have some side effects caused by changing the order of the objects stored in the GEMGeometry object.
Or a new method could be added to GEMGeometry:
const std::vector<GEMSuperChamber*>& GEMGeometry::superChambersWriteable();
to clearly allow writing to super chambers after they have been added.
This PR can go in as-is, but it would be good to consider whether this special pointer list is really the best approach.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, that's a good point. If we move this to after adding to the ring, we can avoid changing the superchambers after adding to theGeometry. I think the ordering should only make a difference in the GEMRing, but this isn't affected by this change.

Copy link
Contributor

Choose a reason for hiding this comment

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

Adding the super chambers to theGeometry after they are modifed makes more sense. As long as there is no side effect from changing the order that the eta parts, GEM chambers, and super chambers are added, this approach is better.

}
GEMChamber *gemChamber = buildChamber(fv, detIdCh);

Expand Down Expand Up @@ -102,8 +103,7 @@ GEMGeometryBuilderFromDDD::build( GEMGeometry& theGeometry,
if (!loopExecuted) delete gemChamber;
}

auto& superChambers(theGeometry.superChambers());
// construct the regions, stations and rings.
// construct the regions, stations and rings.
for (int re = -1; re <= 1; re = re+2) {
GEMRegion* region = new GEMRegion(re);
for (int st=1; st<=GEMDetId::maxStationId; ++st) {
Expand All @@ -113,15 +113,15 @@ GEMGeometryBuilderFromDDD::build( GEMGeometry& theGeometry,
station->setName(name);
for (int ri=1; ri<=1; ++ri) {
GEMRing* ring = new GEMRing(re, st, ri);
for (auto sch : superChambers){
GEMSuperChamber* superChamber = const_cast<GEMSuperChamber*>(sch);
for (auto superChamber : superChambers){
const GEMDetId detId(superChamber->id());
if (detId.region() != re || detId.station() != st || detId.ring() != ri) continue;

superChamber->add( theGeometry.chamber(GEMDetId(detId.region(),detId.ring(),detId.station(),1,detId.chamber(),0)));
superChamber->add( theGeometry.chamber(GEMDetId(detId.region(),detId.ring(),detId.station(),2,detId.chamber(),0)));

ring->add(superChamber);
theGeometry.add(superChamber);
LogDebug("GEMGeometryBuilderFromDDD") << "Adding super chamber " << detId << " to ring: "
<< "re " << re << " st " << st << " ri " << ri << std::endl;
}
Expand Down