Ticket #9118 (closed: fixed)

Opened 7 years ago

Last modified 5 years ago

Refactor and simplify IMDDimensionFactory

Reported by: Russell Taylor Owned by: Russell Taylor
Priority: major Milestone: Release 3.2
Component: Framework Keywords:
Cc: Blocked By:
Blocking: #8487 Tester: Owen Arnold

Description

In #8487 I've been trying to eliminate memory leaks owing to Poco::XML objects not being released. I've been unable to complete this in regard to IMDDimensionFactory and its clients because it is written to take ownership of an Element that is just part of a Document that the client creates.

Looking at the 2 places IMDDimension is used (LoadMD.cpp & MDGeometryXMLParser.cpp) in both cases the factory is created on one line and then used on the next line to generate the IMDDimension object. After that it's finished with so there's no reason it needs to be a full class that holds onto XML-derived objects or anything else - it just needs to be a pair of static methods, one that consumes a string and one a Poco::XML::Element (one will use the other) and both of which give back an IMDDimension object.

Change History

comment:1 Changed 7 years ago by Nick Draper

  • Status changed from new to assigned

comment:2 Changed 7 years ago by Russell Taylor

  • Status changed from assigned to inprogress

Re #9118. Remove unused code.

Changeset: 672fef93ae577b3e5237aaed5e7bdf53d1e1692f

comment:3 Changed 7 years ago by Russell Taylor

Re #9118. Update test to use factory in way I expect it to end up.

Obviously, this won't compile until I've refactored the class itself.

Changeset: 271efdf72040a7c9bc1635b83a19637dd27edfed

comment:4 Changed 7 years ago by Russell Taylor

Re #9118. Remove unused inclusions of IMDDimensionFactory.

Changeset: 6229cdfcd13b7f77fbce21676babd7cde361b247

comment:5 Changed 7 years ago by Russell Taylor

Re #9118. Refactor and greatly simplify IMDDimensionFactory.

It's no longer a class, just 3 non-member functions that create an IMDDimension object (actually an MDHistoDimension object, as before) given an input XML string or a Poco::XML::Element. It no longer retains ownership of any Poco::XML object, which will allow me to handle the previously unreleased memory issues.

Changeset: afb838529789bc391ff6f00cb2fe97b669ce17be

comment:6 Changed 7 years ago by Russell Taylor

Re #9118. Fix narrowing conversion compiler warning the long way.

Rather than resorting to a static_cast, it seems like the method over on the VATES side should be returning a coord_t (float) to align it with the IMDDimension that it relates to.

Changeset: 539299601b8efc31411c6b8e398b0cf8d8054c75

comment:7 Changed 7 years ago by Russell Taylor

Re #9118. Correct tests to fix builds.

Changeset: 77ad2bf1d2dddd3ddf961a7601d9530f35edd5e4

comment:8 Changed 7 years ago by Russell Taylor

Re #9118. Protect against malformed XML.

I added a bunch of tests for the XML string not being as expected and then refactored the code to protect against that.

Changeset: 84af34a7bb6fa19b7f37e23ec37fba64dde6ff87

comment:9 Changed 7 years ago by Russell Taylor

  • Status changed from inprogress to verify
  • Resolution set to fixed

Owen would be a good tester for this.

This mainly has to be tested by code inspection. IMDDimensionFactory has been collapsed down to 3 free functions that hand back an IMDDimension_sptr given input XML as a string or Poco Element. The tests have been modified and expanded. There are no leaks due to Poco XML usage (or anything else) within this class, but note that I haven't yet cleaned up the client classes - that will happen in #8487.

comment:10 Changed 7 years ago by Owen Arnold

  • Status changed from verify to verifying
  • Tester set to Owen Arnold

comment:11 Changed 7 years ago by Owen Arnold

  • Status changed from verifying to closed

Merge remote-tracking branch 'origin/feature/9118_simplify_imddimensionfactory'

Full changeset: 686095330a41e507f4404d2c68cd7929398912bc

comment:12 Changed 7 years ago by Owen Arnold

Good test coverage and additional checking.

comment:13 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 9961

Note: See TracTickets for help on using tickets.