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

Update trigger selection #4774

Merged
merged 3 commits into from
Jul 24, 2014

Conversation

borrello
Copy link
Contributor

Improved the trigger selection to be used for online DQM application

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @borrello for CMSSW_7_1_X.

Update trigger selection

It involves the following packages:

DQMServices/StreamerIO

@ojeda, @danduggan, @rovere, @cmsbuild, @nclopezo, @deguio, @Degano can you please review it and eventually sign? Thanks.
@barvic this is something you requested to watch as well.
You can sign-off by replying to this message having '+1' in the first line of your reply.
You can reject by replying to this message having '-1' in the first line of your reply.
@nclopezo, @ktf you are the release manager for this.
You can merge this pull request by typing 'merge' in the first line of your comment.

@deguio
Copy link
Contributor

deguio commented Jul 24, 2014

+1
[online only]

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs unless changes or unless it breaks tests. @nclopezo, @ktf can you please take care of it?

@cmsbuild
Copy link
Contributor

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?

@davidlange6
Copy link
Contributor

Looks like several memory leaks in this code. Maybe worth an investigation for the future

davidlange6 added a commit that referenced this pull request Jul 24, 2014
@davidlange6 davidlange6 merged commit efe02fa into cms-sw:CMSSW_7_1_X Jul 24, 2014
@cmsbuild
Copy link
Contributor

@dmitrijus
Copy link
Contributor

Hi David,

can you elaborate? I don't see any obvious leaks, but I will fix them if you point me in the right direction.

Dmitrijus

@davidlange6
Copy link
Contributor

I'm just counting the ratio of new to delete.. maybe all of the news are deleted by something else, but I doubt it in all cases.

david

On Jul 24, 2014, at 1:42 PM, cmsbuild notifications@github.com
wrote:

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_1_X IBs unless changes (tests are also fine). @nclopezo, @ktf can you please take care of it?


Reply to this email directly or view it on GitHub.

@dmitrijus
Copy link
Contributor

Hi,

we use shared_ptr, Laura didn't make it clear enough, but I've changed it:

-  typedef boost::shared_ptr<TriggerSelector> TriggerSelectorPtr;
and
-  TriggerSelectorPtr eventSelector_;
+  boost::shared_ptr<TriggerSelector> eventSelector_;

I will make a separate pull request somewhat next week (with other fixes too).

Dmitrijus

@Dr15Jones
Copy link
Contributor

Please use std::shared_ptr instead of boost::shared_ptr. We are trying to remove our use of boost in preference to C++11 equivalents.

@dmitrijus
Copy link
Contributor

I know, but this is 7_1_X,

7_2_X has already migrated all the code to use std::shared_ptr,
And I would love if someone would do s/boost::shared_ptr/std::shared_ptr/g for 7_1_X,
this would simplify our forward porting a lot.

But now I cannot just switch to std::shared_ptr,
due to inheritance from InputSource class (which still uses boost).

Dmitrijus

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants