Mapping functions: Return bare pointer -> shared pointer

[Nick Draper]

We should replace with shared pointers throughout

[Nick Draper]

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.

[Stuart Campbell]

I don't remember adding anything - can you be more specific to where in the code to look?

[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.

[Russell Taylor]

Suggest waiting for #6191 to avoid wasting time.

[Stuart Campbell]

bumping to next release as blocking ticket #6191 is delayed.

[Russell Taylor]

The two "getWorkspaceIndexTo..." methods have gone under #6198 (commits [873db8e8c] & [757dc57b]).

[Nick Draper]

Moved to the Backlog after R2.6

[Stuart Campbell]

Because Russell asked for it.

[Russell Taylor]

The Coverity scan showed yet another leak surrounding the use of these functions (in MantidWSIndexDialog). Let's just get it fixed.

[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

[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

[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

[Russell Taylor]

Re #6198. Fix clients of mapping methods in Crystal package.

Changeset: c25a88b42b87bf415ef896bf7fcb0621b17090bb

[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

[Russell Taylor]

Fix previously-leaking usage of getSpectrumToWorkspaceIndexMap.

This is the one that the Coverity scan highlighted. Re #6198.

Changeset: c96632afb717277197b0016a2950a8b83b0ac63f

[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

[Russell Taylor]

Re #6198. Fix usages of mapping methods in tests.

Changeset: b6cad4120186ff174ca027fcf977f0029578a5b7

[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.

[Russell Taylor]

Re #6198. Fix for crashing unit test on Mac.

Changeset: 8c931453f53bbd32c26c8c90d26ec60158c7fb89

[Russell Taylor]

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.

[Roman Tolchenov]

Merge remote-tracking branch 'origin/feature/6198_mapping_functions_remove_bare_pointers'

[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

[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

[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

[Russell Taylor]

Re #6198. Fix clients of mapping methods in Crystal package.

Changeset: 8a44c333e947a1f4e831b74047781789a561ddeb

[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

[Russell Taylor]

Fix previously-leaking usage of getSpectrumToWorkspaceIndexMap.

This is the one that the Coverity scan highlighted. Re #6198.

Changeset: e30fb507baf4fd0fed58d8b0ba71048276e691cd

[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

[Russell Taylor]

Re #6198. Fix usages of mapping methods in tests.

Changeset: 8aacb1e0c25b284c4db46843007f1ff7e5bc6234

[Russell Taylor]

Re #6198. Fix for crashing unit test on Mac.

Changeset: 85dcbe2cb34a7240230e68f417ab4025caa49836

[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

[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

[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

[Russell Taylor]

Re #6198. Fix clients of mapping methods in Crystal package.

Changeset: 8a44c333e947a1f4e831b74047781789a561ddeb

[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

[Russell Taylor]

Fix previously-leaking usage of getSpectrumToWorkspaceIndexMap.

This is the one that the Coverity scan highlighted. Re #6198.

Changeset: e30fb507baf4fd0fed58d8b0ba71048276e691cd

[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

[Russell Taylor]

Re #6198. Fix usages of mapping methods in tests.

Changeset: 8aacb1e0c25b284c4db46843007f1ff7e5bc6234

[Russell Taylor]

Re #6198. Fix for crashing unit test on Mac.

Changeset: 85dcbe2cb34a7240230e68f417ab4025caa49836

[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

[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

[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

[Russell Taylor]

Re #6198. Fix clients of mapping methods in Crystal package.

Changeset: 8a44c333e947a1f4e831b74047781789a561ddeb

[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

[Russell Taylor]

Fix previously-leaking usage of getSpectrumToWorkspaceIndexMap.

This is the one that the Coverity scan highlighted. Re #6198.

Changeset: e30fb507baf4fd0fed58d8b0ba71048276e691cd

[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

[Russell Taylor]

Re #6198. Fix usages of mapping methods in tests.

Changeset: 8aacb1e0c25b284c4db46843007f1ff7e5bc6234

[Russell Taylor]

Re #6198. Fix for crashing unit test on Mac.

Changeset: 85dcbe2cb34a7240230e68f417ab4025caa49836

[Stuart Campbell]

This ticket has been transferred to github issue ​7044