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

Don't return pointer to principal #641

Closed

Conversation

wmtan
Copy link
Contributor

@wmtan wmtan commented Aug 27, 2013

Functions that read a run, lumi, or an event principal fill in the principal that is passed in by reference. These functions no longer create the principal. Therefore, there is no need for them to return a pointer to the principal.
With this pull request, these functions now return void or bool, as appropriate.
Also, the functions readAndCacheRun and readAndCacheLumi were modified to fill in the principal rather than creating it, and were renamed readRun and readLuminosityBlock respectively, because the caching of the principal
has been moved outside these functions.

The changes outside the framework are just minimal adaptions to the framework changes.

These changes have been tested by runTheMatrix.py -s and unit tests. The full runTheMatrix.py is in progress.

@cmsbuild
Copy link
Contributor

A new Pull Request was created by @wmtan for CMSSW_7_0_X.

Don't return pointer to principal

It involves the following packages:

EventFilter/StorageManager
FWCore/Sources
IORawData/DaqSource
DQMServices/FwkIO
IOPool/Input
FWCore/Framework
FWCore/Integration
GeneratorInterface/LHEInterface
IOPool/Streamer

@vciulli, @Dr15Jones, @rovere, @deguio, @ktf can you please review it and eventually sign? Thanks.
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.

@cmsbuild
Copy link
Contributor

The following categories have been signed by @rovere: DQM

@cms-git-dqm

boost::shared_ptr<RunPrincipal> rp(new RunPrincipal(runAuxiliary(), productRegistry_, processConfiguration(), &historyAppender,0));
callWithTryCatchAndPrint<boost::shared_ptr<RunPrincipal> >( [this,&rp](){ return readRun_(rp); }, "Calling InputSource::readRun_" );
return rp;
callWithTryCatchAndPrint<void>( [this,&runPrincipal](){ readRun_(runPrincipal); }, "Calling InputSource::readRun_" );
Copy link
Contributor

Choose a reason for hiding this comment

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

FYI technically, passing runPrincipal by value is fine here since it can't be modified.

@Dr15Jones
Copy link
Contributor

+1

@ktf
Copy link
Contributor

ktf commented Aug 28, 2013

@nclopezo please test.

@nclopezo
Copy link
Contributor

Hi,

I ran the tests on top of CMSSW_6_2_X_2013-08-28-0200, all tests passed

The logs are here:
https://cmssdt.cern.ch/jenkins/job/Pull-Request-Integration/ARCHITECTURE=slc5_amd64_gcc472/303/consoleFull

and the artifacts here:

https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/303/

@wmtan
Copy link
Contributor Author

wmtan commented Aug 29, 2013

This pull request will possibly be no longer automatically mergeable because EventFilter/StorageManager has been removed from the repository. If so, don't waste any time on this one, because I will be submitting another pull request soon that supersedes this one, and, at that time, I will close this one if it is still unmerged. On the other hand, there is no harm in merging this one now if it is easy to do so.

@Dr15Jones
Copy link
Contributor

I believe you meant to say you tested upon CMSSW_7_0_X_2013-08-28-0200 not 6_2_X.

@nclopezo
Copy link
Contributor

Hi @Dr15Jones,

Sorry, I copy pasted the wrong thing, it was CMSSW_7_0_X_2013-08-28-0200, as you an see in the logs.

@wmtan
Copy link
Contributor Author

wmtan commented Aug 30, 2013

I am closing this request because it is probably no longer automatically mergeable, and it is superseded by pull request 681.

@wmtan wmtan closed this Aug 30, 2013
@wmtan wmtan deleted the NoPointerReturnedFromReadFunctions branch August 30, 2013 22:43
jpata pushed a commit to jpata/cmssw that referenced this pull request Jun 9, 2016
…lphatpt

add options in the alphat calculation
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.

5 participants