Ticket #6198 (closed: fixed)
Mapping functions: Return bare pointer -> shared pointer
Reported by: | Nick Draper | Owned by: | Russell Taylor |
---|---|---|---|
Priority: | critical | Milestone: | Release 3.0 |
Component: | Framework | Keywords: | Maintenance |
Cc: | Blocked By: | #6191 | |
Blocking: | Tester: | Roman Tolchenov |
Description
We should replace with shared pointers throughout
Change History
comment:2 Changed 8 years ago by Stuart Campbell
I don't remember adding anything - can you be more specific to where in the code to look?
comment:3 Changed 8 years ago by Russell Taylor
The methods mentioned in duplicate ticket #6481 are (in MatrixWorkspace):
index2spec_map * getWorkspaceIndexToSpectrumMap() const; spec2index_map * getSpectrumToWorkspaceIndexMap() const; index2detid_map * getWorkspaceIndexToDetectorIDMap() const; detid2index_map * getDetectorIDToWorkspaceIndexMap( bool throwIfMultipleDets ) const;
As I said over there the first two will go away under #6191.
comment:5 Changed 8 years ago by Russell Taylor
- Blocked By 6191 added
Suggest waiting for #6191 to avoid wasting time.
comment:7 Changed 7 years ago by Stuart Campbell
- Milestone changed from Release 2.5 to Release 2.6
bumping to next release as blocking ticket #6191 is delayed.
comment:8 Changed 7 years ago by Nick Draper
- Keywords Maintenance added
- Priority changed from blocker to critical
comment:9 Changed 7 years ago by Russell Taylor
The two "getWorkspaceIndexTo..." methods have gone under #6198 (commits [873db8e8c] & [757dc57b]).
comment:11 Changed 7 years ago by Nick Draper
- Milestone changed from Release 2.6 to Backlog
Moved to the Backlog after R2.6
comment:12 Changed 7 years ago by Stuart Campbell
- Owner changed from Stuart Campbell to Russell Taylor
Because Russell asked for it.
comment:13 Changed 7 years ago by Russell Taylor
- Status changed from new to inprogress
- Milestone changed from Backlog to Release 3.0
The Coverity scan showed yet another leak surrounding the use of these functions (in MantidWSIndexDialog). Let's just get it fixed.
comment:14 Changed 7 years ago by Russell Taylor
Re #6198. Just return the maps by value.
In C++11, map has a move constructor so returning by value isn't expensive. Actually, even on the non-C++11 Mac tests show that the RVO saves us.
Now to fix all the client code...
Changeset: 1f525ef0258912ec82af95695d3a53629aa3f99e
comment:15 Changed 7 years ago by Russell Taylor
Re #6198. Fix usages of mapping methods for value return.
Notes:
- 4 classes weren't deleting the returned pointer
- more were not exception safe
- some changes rely on the move assignment operator for efficiency.
Changeset: d0f54814f44ad884aa6bc484d7380d10ad2b4116
comment:16 Changed 7 years ago by Russell Taylor
Re #6198. Fix clients of mapping methods in Algorithms package.
(At least 5 more places that weren't deleting the returned map.)
Changeset: 7c7bbfae6393b73dfbf6a488d3c0605f4df132df
comment:17 Changed 7 years ago by Russell Taylor
Re #6198. Fix clients of mapping methods in Crystal package.
Changeset: c25a88b42b87bf415ef896bf7fcb0621b17090bb
comment:18 Changed 7 years ago by Russell Taylor
Re #6198. Fix last use of mapping method in Framework.
We're relying on the move assignment operator here for efficiency, so things will be a little slower on the Mac until it gets C++11 libs. But we're not leaking the thing like we were before.
Changeset: b49330ab6febb0460cb017447ea9e54de9f0bcd5
comment:19 Changed 7 years ago by Russell Taylor
Fix previously-leaking usage of getSpectrumToWorkspaceIndexMap.
This is the one that the Coverity scan highlighted. Re #6198.
Changeset: c96632afb717277197b0016a2950a8b83b0ac63f
comment:20 Changed 7 years ago by Russell Taylor
Re #6198. Update InstrumentView usage of mapping method.
I restored the documented exception throwing behaviour after checking that every place that calls the method catches the exception. Not sure when/why that behaviour was changed, but this avoids the risk of accessing an invalid iterator.
Changeset: f09ccaf65f7ec33a28ae630218c024f967812a1a
comment:21 Changed 7 years ago by Russell Taylor
Re #6198. Fix usages of mapping methods in tests.
Changeset: b6cad4120186ff174ca027fcf977f0029578a5b7
comment:22 Changed 7 years ago by Russell Taylor
On the Mac, EditInstrumentGeometryTest is crashing inside the map::find method at line 290 of EditInstrumentGeometry. I cannot for the moment fathom why.
comment:23 Changed 7 years ago by Russell Taylor
Re #6198. Fix for crashing unit test on Mac.
Changeset: 8c931453f53bbd32c26c8c90d26ec60158c7fb89
comment:24 Changed 7 years ago by Russell Taylor
- Status changed from inprogress to verify
- Resolution set to fixed
Tester: Branch is feature/6198_mapping_functions_remove_bare_pointers
Despite the ticket description, I went for returning the created map by value. In the vast majority of cases this is just fine everywhere because we get the Named Return Value Optimization. There are just a couple of cases where the methods are called and assigned to an existing variable. With C++11, return-by-value is still fine as std::map has a move assignment operator. Without a C++11 library (currently the Mac) we will suffer a copy in these cases, but I'm asserting that we can live with that until the Mac catches up.
Test by inspecting code and observing that all tests are passing.
comment:25 Changed 7 years ago by Roman Tolchenov
- Status changed from verify to verifying
- Tester set to Roman Tolchenov
comment:26 Changed 7 years ago by Roman Tolchenov
- Status changed from verifying to closed
Merge remote-tracking branch 'origin/feature/6198_mapping_functions_remove_bare_pointers'
comment:27 Changed 7 years ago by Russell Taylor
Re #6198. Just return the maps by value.
In C++11, map has a move constructor so returning by value isn't expensive. Actually, even on the non-C++11 Mac tests show that the RVO saves us.
Now to fix all the client code...
Changeset: 419a24854a0bcdd1913e0a39b316845406bce967
comment:28 Changed 7 years ago by Russell Taylor
Re #6198. Fix usages of mapping methods for value return.
Notes:
- 4 classes weren't deleting the returned pointer
- more were not exception safe
- some changes rely on the move assignment operator for efficiency.
Changeset: 59e1811525e3e61e990fc9768bf73e9dd8716019
comment:29 Changed 7 years ago by Russell Taylor
Re #6198. Fix clients of mapping methods in Algorithms package.
(At least 5 more places that weren't deleting the returned map.)
Changeset: 553f7fe05e9a6d4aa8b618ecb8a623fd1c33a251
comment:30 Changed 7 years ago by Russell Taylor
Re #6198. Fix clients of mapping methods in Crystal package.
Changeset: 8a44c333e947a1f4e831b74047781789a561ddeb
comment:31 Changed 7 years ago by Russell Taylor
Re #6198. Fix last use of mapping method in Framework.
We're relying on the move assignment operator here for efficiency, so things will be a little slower on the Mac until it gets C++11 libs. But we're not leaking the thing like we were before.
Changeset: 14b9d0b7003b0440453033fbeaef2d1e291f77b4
comment:32 Changed 7 years ago by Russell Taylor
Fix previously-leaking usage of getSpectrumToWorkspaceIndexMap.
This is the one that the Coverity scan highlighted. Re #6198.
Changeset: e30fb507baf4fd0fed58d8b0ba71048276e691cd
comment:33 Changed 7 years ago by Russell Taylor
Re #6198. Update InstrumentView usage of mapping method.
I restored the documented exception throwing behaviour after checking that every place that calls the method catches the exception. Not sure when/why that behaviour was changed, but this avoids the risk of accessing an invalid iterator.
Changeset: c67e1e850528a02136aafc36b2f99a428cc4c574
comment:34 Changed 7 years ago by Russell Taylor
Re #6198. Fix usages of mapping methods in tests.
Changeset: 8aacb1e0c25b284c4db46843007f1ff7e5bc6234
comment:35 Changed 7 years ago by Russell Taylor
Re #6198. Fix for crashing unit test on Mac.
Changeset: 85dcbe2cb34a7240230e68f417ab4025caa49836
comment:36 Changed 7 years ago by Russell Taylor
Re #6198. Just return the maps by value.
In C++11, map has a move constructor so returning by value isn't expensive. Actually, even on the non-C++11 Mac tests show that the RVO saves us.
Now to fix all the client code...
Changeset: 419a24854a0bcdd1913e0a39b316845406bce967
comment:37 Changed 7 years ago by Russell Taylor
Re #6198. Fix usages of mapping methods for value return.
Notes:
- 4 classes weren't deleting the returned pointer
- more were not exception safe
- some changes rely on the move assignment operator for efficiency.
Changeset: 59e1811525e3e61e990fc9768bf73e9dd8716019
comment:38 Changed 7 years ago by Russell Taylor
Re #6198. Fix clients of mapping methods in Algorithms package.
(At least 5 more places that weren't deleting the returned map.)
Changeset: 553f7fe05e9a6d4aa8b618ecb8a623fd1c33a251
comment:39 Changed 7 years ago by Russell Taylor
Re #6198. Fix clients of mapping methods in Crystal package.
Changeset: 8a44c333e947a1f4e831b74047781789a561ddeb
comment:40 Changed 7 years ago by Russell Taylor
Re #6198. Fix last use of mapping method in Framework.
We're relying on the move assignment operator here for efficiency, so things will be a little slower on the Mac until it gets C++11 libs. But we're not leaking the thing like we were before.
Changeset: 14b9d0b7003b0440453033fbeaef2d1e291f77b4
comment:41 Changed 7 years ago by Russell Taylor
Fix previously-leaking usage of getSpectrumToWorkspaceIndexMap.
This is the one that the Coverity scan highlighted. Re #6198.
Changeset: e30fb507baf4fd0fed58d8b0ba71048276e691cd
comment:42 Changed 7 years ago by Russell Taylor
Re #6198. Update InstrumentView usage of mapping method.
I restored the documented exception throwing behaviour after checking that every place that calls the method catches the exception. Not sure when/why that behaviour was changed, but this avoids the risk of accessing an invalid iterator.
Changeset: c67e1e850528a02136aafc36b2f99a428cc4c574
comment:43 Changed 7 years ago by Russell Taylor
Re #6198. Fix usages of mapping methods in tests.
Changeset: 8aacb1e0c25b284c4db46843007f1ff7e5bc6234
comment:44 Changed 7 years ago by Russell Taylor
Re #6198. Fix for crashing unit test on Mac.
Changeset: 85dcbe2cb34a7240230e68f417ab4025caa49836
comment:45 Changed 7 years ago by Russell Taylor
Re #6198. Just return the maps by value.
In C++11, map has a move constructor so returning by value isn't expensive. Actually, even on the non-C++11 Mac tests show that the RVO saves us.
Now to fix all the client code...
Changeset: 419a24854a0bcdd1913e0a39b316845406bce967
comment:46 Changed 7 years ago by Russell Taylor
Re #6198. Fix usages of mapping methods for value return.
Notes:
- 4 classes weren't deleting the returned pointer
- more were not exception safe
- some changes rely on the move assignment operator for efficiency.
Changeset: 59e1811525e3e61e990fc9768bf73e9dd8716019
comment:47 Changed 7 years ago by Russell Taylor
Re #6198. Fix clients of mapping methods in Algorithms package.
(At least 5 more places that weren't deleting the returned map.)
Changeset: 553f7fe05e9a6d4aa8b618ecb8a623fd1c33a251
comment:48 Changed 7 years ago by Russell Taylor
Re #6198. Fix clients of mapping methods in Crystal package.
Changeset: 8a44c333e947a1f4e831b74047781789a561ddeb
comment:49 Changed 7 years ago by Russell Taylor
Re #6198. Fix last use of mapping method in Framework.
We're relying on the move assignment operator here for efficiency, so things will be a little slower on the Mac until it gets C++11 libs. But we're not leaking the thing like we were before.
Changeset: 14b9d0b7003b0440453033fbeaef2d1e291f77b4
comment:50 Changed 7 years ago by Russell Taylor
Fix previously-leaking usage of getSpectrumToWorkspaceIndexMap.
This is the one that the Coverity scan highlighted. Re #6198.
Changeset: e30fb507baf4fd0fed58d8b0ba71048276e691cd
comment:51 Changed 7 years ago by Russell Taylor
Re #6198. Update InstrumentView usage of mapping method.
I restored the documented exception throwing behaviour after checking that every place that calls the method catches the exception. Not sure when/why that behaviour was changed, but this avoids the risk of accessing an invalid iterator.
Changeset: c67e1e850528a02136aafc36b2f99a428cc4c574
comment:52 Changed 7 years ago by Russell Taylor
Re #6198. Fix usages of mapping methods in tests.
Changeset: 8aacb1e0c25b284c4db46843007f1ff7e5bc6234
comment:53 Changed 7 years ago by Russell Taylor
Re #6198. Fix for crashing unit test on Mac.
Changeset: 85dcbe2cb34a7240230e68f417ab4025caa49836
comment:54 Changed 5 years ago by Stuart Campbell
This ticket has been transferred to github issue 7044
These are the methods that I think Stuart added to create vectors of detector_id given workspaces indexes and the other similar methods.
There was some concern that they returned bare pointers and that the responsibility of deleting the object fell to the calling method. The developers at the dev meeting agreed that this should be altered to return shared pointers to reduce the liklihood of memory leaks.