Ticket #9949 (closed: fixed)

Opened 6 years ago

Last modified 5 years ago

Use shared pointer in LoadEventNexus::runLoadNexusLogs

Reported by: Wenduo Zhou Owned by: Wenduo Zhou
Priority: critical Milestone: Release 3.3
Component: Framework Keywords:
Cc: petersonpf@… Blocked By:
Blocking: Tester: Martyn Gigg

Description

Method runLoadNexusLogs of algorithm LoadEventNexus uses a pointer, which is reported as a resource leak by coverity. Make it a shared pointer to fix the issue.

Change History

comment:1 Changed 6 years ago by Wenduo Zhou

Refs #9949. Replace pointer by reference.

Changeset: 3a20748b7ba3462542a6ccd52c894b6c360a451f

comment:2 Changed 6 years ago by Wenduo Zhou

  • Status changed from new to assigned

comment:3 Changed 6 years ago by Wenduo Zhou

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

comment:4 Changed 6 years ago by Roman Tolchenov

  • Status changed from verify to verifying
  • Tester set to Roman Tolchenov

comment:5 Changed 6 years ago by Roman Tolchenov

  • Status changed from verifying to reopened
  • Resolution fixed deleted

It's not the issue coverity reported. LoadEventNexus::runLoadNexusLogs(...) allocates an object of type BankPulseTimes and returns it. The code in LoadTOFRawNexus doesn't assign the returned address to any variable and of course doesn't free the memory.

comment:6 Changed 6 years ago by Wenduo Zhou

  • Status changed from reopened to inprogress

Fixed the issue described by Roman. Refs #9949.

Changeset: 0960cef370f3b68279bc7a8c27e06254c8660d8e

comment:7 Changed 6 years ago by Wenduo Zhou

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

comment:8 Changed 6 years ago by Martyn Gigg

  • Status changed from verify to verifying
  • Tester changed from Roman Tolchenov to Martyn Gigg

comment:9 Changed 6 years ago by Martyn Gigg

  • Status changed from verifying to reopened
  • Resolution fixed deleted

I think we can fix this in a better way.

The ticket title states that runLoadNexusLogs should return a shared_ptr. I agree with that, it would avoid having to worry about the deletion of the pulse times.

My second comment is that LoadTOFRawNexus doesn't actually use the pulse times at all so it seems like a waste allocating them in the first place. What about adding an additional flag to runLoadNexusLogs, something like loadPulseTimes, which defaults to true so the behaviour is like it was before?

comment:10 Changed 6 years ago by Wenduo Zhou

  • Status changed from reopened to inprogress

Modified to avoid creating unnecessary return object. Refs #9949.

Changeset: 803d0db34f8c84f95e6dd0d10d8ad2efa1c801f0

comment:11 Changed 6 years ago by Wenduo Zhou

Replaced by shared pointers. Refs #9949.

Changeset: 7f8fd473a0d7cfece09c3a78f29439abb514db68

comment:12 Changed 6 years ago by Wenduo Zhou

Changed method argument type to const. Refs #9949.

Changeset: eaf3935665123c354964a51cdae373ff7ece78d3

comment:13 Changed 6 years ago by Wenduo Zhou

Tried to fix mac build error. Refs #9949.

Changeset: dbbf14fab2ec2f9fa2f2dd4e2fa797a7c4cc8b6f

comment:14 Changed 6 years ago by Wenduo Zhou

Fixed doxygen error. Refs #9949.

Changeset: 486764b89b311c36451b5de5aa2fb2b6b214c2db

comment:15 Changed 6 years ago by Wenduo Zhou

Movef runLoadNexusLog back to LoadEventNexus. Refs #9949.

Changeset: 2695bb5d43ee8815ac1f7edbd86af62e53e94f27

comment:16 Changed 6 years ago by Wenduo Zhou

Fixed doxygen error. Refs #9949.

Changeset: 01e64fb46ccf7144b8b21742fd55c3b98e40aaab

comment:17 Changed 6 years ago by Wenduo Zhou

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

comment:18 Changed 6 years ago by Martyn Gigg

  • Status changed from verify to verifying

comment:19 Changed 6 years ago by Martyn Gigg

  • Status changed from verifying to closed

I'm happier with the changes now.

comment:20 Changed 6 years ago by Martyn Gigg

Merge remote-tracking branch 'origin/bugfix/9949_coverity_share_ptr'

Full changeset: 6fff5cbccfe7ece77b2efe9036717120e406c194

comment:21 Changed 5 years ago by Stuart Campbell

This ticket has been transferred to github issue 10791

Note: See TracTickets for help on using tickets.