Ticket #9949 (closed: fixed)
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: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: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
Refs #9949. Replace pointer by reference.
Changeset: 3a20748b7ba3462542a6ccd52c894b6c360a451f