Project

General

Profile

Support #1089

SciFiHit usage

Added by Dobbs, Adam over 10 years ago. Updated over 10 years ago.

Status:
Rejected
Priority:
Normal
Assignee:
Rogers, Chris
Category:
Tracker
Target version:
Future MAUS release
Start date:
05 August 2012
Due date:
% Done:

0%

Estimated time:
Workflow:
New Issue

Description

Currently writing unit tests for SciFiEvent in the MAUS namespace, which uses the SciFiHit derived from Hit already provided in DataStructure. Am experiencing a seg fault when I try to access the Hits in the Event, but not when I do this for any of the other data members (digits, clusters, etc). Some example code is attached which is testing the SciFiEvent copy constructor. This code runs and passes fine, provided line 33 is commented out:

EXPECT_EQ(evt2.hits()[0]->GetChannelId()->GetFiberNumber(), 0);

hits() returns a SciFiHitPArray, defined as

typedef std::vector<SciFiHit*> SciFiHitPArray;

in Hit.hh.

I also attached the SciFiEvent class files.

Am I not using the SciFiHit class / typedef correctly, or could this be a real issue?

Thanks!

Files

SciFiEventTestPart.cc (1.87 KB) SciFiEventTestPart.cc Dobbs, Adam, 05 August 2012 02:48
SciFiEvent.hh (4.03 KB) SciFiEvent.hh Dobbs, Adam, 05 August 2012 02:48
SciFiEvent.cc (4.81 KB) SciFiEvent.cc Dobbs, Adam, 05 August 2012 02:48
#1

Updated by Dobbs, Adam over 10 years ago

In fact, I can illustrate things more clearly. The following:

SciFiHit *hit = new SciFiHit();
  std::cerr << hit->GetChannelId()->GetFiberNumber() << std::endl;

Generates a seg fault. Even if I am getting the usage wrong, a seg fault is probably not a happy place for our code to go in that event...

#2

Updated by Dobbs, Adam over 10 years ago

Ok, think I have figured this out. So before using anything within _channel_id, it should be set first, along the lines of:

SciFiChannelId my_id;
  SciFiHit my_hit;
  my_hit.SetChannelId(my_id);

which should then allow access as expected.

Is this a bit dangerous? Perhaps the _channel_id could be newed in the Hit default constructor: 

template &lt;class ChannelId&gt;
  Hit&lt;ChannelId&gt;::Hit() : _track_id(0), _particle_id(0), _energy(0), _charge(0), _time(0),
               _energy_deposited(0), _position(0, 0, 0), _momentum(0, 0, 0),
               _channel_id(NULL) {
    _channel_id = new ChannelId();
  }
#3

Updated by Rogers, Chris over 10 years ago

  • Status changed from Open to Rejected
  • Target version set to Future MAUS release

Sorry, it's up to the user to check for NULL values. You can call

  my_hit.SetChannelId(NULL);

and might even want to do this sometime. You just need to be aware that it's a pointer, it may be NULL. What I guarantee is that a pointer which is not assigned is always NULL (or it's a bug). I expect others to do the same (for example calling delete on my classes - they will attempt to delete memory they own if it is not assigned to NULL).

#4

Updated by Dobbs, Adam over 10 years ago

Hi Chris,

Thanks for the reply. Agree, its fair for users to be expected to check pointers before they use them. Does that mean though that newing a pointer in a class constructor should be always forbidden? I think in this instance it could be helpful, without any particular drawbacks. For example, say I instantiate a SciFiHit in the current scheme:

SciFiHit *hit = new SciFiHit();
  SciFiChannelId *sf_id = new SciFiChannelId();
  hit->SetChannelId(sf_id);

For an array of hits it becomes more complicated still. Previously all we needed was the first line, as we would if there was a new in the constructor. Also, consider

delete hit;

The user now has to go into the .cc of Hit to figure out the status of sf_id. Similarly if sf_id is deleted. If think the upshot is the code as it stands is harder to use in practice than the old system where we implemented SciFiHit as an independent class (not that I want to go back there, i just don't want to lose out on the implementation).

Another approach might be to implement SciFiHit as a class which inherits from Hit, rather than a typedef - template.

Its your call, but I wanted to put my case, before I start trying to code this :)

Also available in: Atom PDF