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

Add EBPMT for PMT pairing in EventBuilding #332

Open
wants to merge 1 commit into
base: Application
Choose a base branch
from

Conversation

fengyvoid
Copy link
Contributor

@fengyvoid fengyvoid commented Jan 20, 2025

EBPMT Tool in EventBuildingV2 tool set.

This is splitted from PR #307, I agree that it's definitely easier to submit tools one by one and not necessary to submit by a whole tool chain. I will also list the changes based on your comments in #307.

Changes based on your comments:

· Can we translate EBPMT.cpp line 386 to english please?
Yes, now it's at 396 - 398

@marc1uk
Copy link
Collaborator

marc1uk commented Feb 24, 2025

RWMRawWaveforms and BRFRawWaveforms (lines 29,30) are both immediately leaked by lines 46,47.
It would be sensible to either check the return of the corresponding Get calls. It may also be worth checking the corresponding pointers are not null before de-referencing them in the following log lines.

Is it worth moving the check on line 76 to line 44; is this just doing needless work if those checks are both false?

i think line 122 should be enclosed with line 121 in { } for line 120 to prevent spurious printouts.

It seems like it may not be intended that the printout in line 125 may result in the insertion of an element into the AlmostCompletedWavefroms map via operator[].
Same for line 137. Note that .at calls will throw in the event that the element doesn't exist - there may be more logic required to avoid that. I'm getting a bit lost with the different potential if conditions. May be better to check & access the map just once and re-use the result.

Would suggest using references on lines 152, 154, 284, 290, 295, 297, 446, 447, 450-453 to avoid copies.

Not sure if the logic prohibits it but would suggest for good practice the use of emplace rather than operator[] and checking the return value second to avoid potentially clobbering previous entries on lines 325, 326, 327.

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