Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Protect against failed cast #162

Merged
merged 1 commit into from
Aug 11, 2022
Merged

Protect against failed cast #162

merged 1 commit into from
Aug 11, 2022

Conversation

gondiaz
Copy link
Contributor

@gondiaz gondiaz commented Apr 21, 2022

In interactive mode, you might want to use DefaultEventAction without DefaultTrackingAction. Since you are in interactive mode, Geant4 will provide a not empty G4TrajectoryContainer, which is a vector of G4VTrajectory. The dowcast fails since Trajectory is a nexus class.

@gondiaz gondiaz requested a review from paolafer April 21, 2022 17:08
@paolafer
Copy link
Contributor

I would say that in the interactive mode there is no need to use DefaultEventAction either, since one is not concerned with how many events are saved in the output file. One could simply use the default actions by not specifying any custom actions.
In fact, if one wanted to save only events with energy deposition above a threshold, and at the same time is not using DefaultTrackingAction, it wouldn't work, because the calculated energy deposited would be always zero. I will document the use of each action better, once the PR on the clean-up of the actions is approved.

@paolafer
Copy link
Contributor

What we could do is, instead of adding a protection, adding a meaningful error message and let the program crash.

@gondiaz
Copy link
Contributor Author

gondiaz commented Apr 22, 2022

Agree. What do you think about placing such an error before the SetUserAction in NexusApp.cc? This would assure that you always choose a compatible set of user actions

@paolafer
Copy link
Contributor

Agree. What do you think about placing such an error before the SetUserAction in NexusApp.cc?

I don't like it vey much, I would prefer to keep the code in the NexusApp class as simple as possible. I would rather put the protection in DefaultEventAction, which is the class that accesses the Trajectory object.

@paolafer
Copy link
Contributor

@gondiaz, what about this PR? Are you going to add the comment to the crash or shall we close it?

@gondiaz
Copy link
Contributor Author

gondiaz commented Jul 26, 2022

At least not in the next two weeks

@paolafer
Copy link
Contributor

At least not in the next two weeks

This doesn't answer my question :-).

@gondiaz
Copy link
Contributor Author

gondiaz commented Jul 26, 2022

The answer is yes then, but I don't know when.
If it is annoying to have this pending, it can be closed. If anyone wants to contribute and finish it, also works for me.

@paolafer
Copy link
Contributor

It's not annoying, I just want to know if you forgot about it or you are going to finish it at some point.

@gondiaz gondiaz force-pushed the trj branch 2 times, most recently from 91e01c5 to 54b53fe Compare August 9, 2022 21:43
// but the trajectories will not cast to Trajectory
Trajectory* trj = dynamic_cast<Trajectory*>((*tc)[0]);
if (trj == nullptr){
G4Exception("[DefaultEventAction]", "EndOfEventAction", FatalException,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the nexus convention, the second argument of G4Exception should be the function, so () should be added.

Trajectory* trj = dynamic_cast<Trajectory*>((*tc)[0]);
if (trj == nullptr){
G4Exception("[DefaultEventAction]", "EndOfEventAction", FatalException,
"Adequate TrackingAction not registered for DefaultEventAction");
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would say explicitly which one is the correct action, something like "DefaultTrackingAction is required when using DefaultEventAction".

Copy link
Contributor

@paolafer paolafer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR adds a useful error message when DefaultTrackingAction is not used together with DefaultEventAction. Approved!

@paolafer paolafer merged commit ec763ad into next-exp:master Aug 11, 2022
@gondiaz gondiaz deleted the trj branch August 11, 2022 15:05
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants