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

Make theVerboseLevel constexpr #7192

Merged

Conversation

Dr15Jones
Copy link
Contributor

The static analyzer was complaining about the file static
theVerboseLevel in SiPixelTemplateSplit. Since the value is never
changed at run time the variable was marked as constexpr.

The static analyzer was complaining about the file static
theVerboseLevel in SiPixelTemplateSplit. Since the value is never
changed at run time the variable was marked as constexpr.
@cmsbuild
Copy link
Contributor

A new Pull Request was created by @Dr15Jones (Chris Jones) for CMSSW_7_4_X.

Make theVerboseLevel constexpr

It involves the following packages:

RecoLocalTracker/SiPixelRecHits

@cmsbuild, @cvuosalo, @nclopezo, @slava77 can you please review it and eventually sign? Thanks.
@makortel, @GiacomoSguazzoni, @rovere, @VinInn, @dkotlins, @gpetruc, @cerati, @threus, @venturia 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.
If you are a L2 or a release manager you can ask for tests by saying 'please test' in the first line of a comment.

@@ -47,7 +47,7 @@
#include "FWCore/MessageLogger/interface/MessageLogger.h"
#define LOGERROR(x) edm::LogError(x)
#define LOGDEBUG(x) LogDebug(x)
static int theVerboseLevel = 2;
static constexpr int theVerboseLevel = 2;
Copy link
Contributor

Choose a reason for hiding this comment

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

does it make sense to be static here? could just be constexpr int theVerboseLevel = 2;

Copy link
Contributor

Choose a reason for hiding this comment

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

I think should be not static and enclosed in anonymous namespace

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Static guarantees the symbol is not exported. I've never checked to see if anonymous namespaces have the same guarantee.

Copy link
Contributor

Choose a reason for hiding this comment

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

that's the role of "anonymous namespace" in C++ (w/r/t static as "local-to-file" in C)

Copy link
Contributor

Choose a reason for hiding this comment

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

@Dr15Jones will you change this or do we stick to static?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Given all the stuff I have on my plate and that we use static much more than unnamed namespaces I'd prefer to keep it as it is.

@slava77
Copy link
Contributor

slava77 commented Jan 16, 2015

@cmsbuild please test

(compile is enough)

@cmsbuild
Copy link
Contributor

The tests are being triggered in jenkins.

@slava77
Copy link
Contributor

slava77 commented Jan 17, 2015

@cmsbuild please test
... well, I was hoping the first time would actually work

@slava77
Copy link
Contributor

slava77 commented Jan 17, 2015

Looks like jenkins stepped on itself

https://cmssdt.cern.ch/jenkins/job/auto-trigger-pr-tests/5081/console

@slava77
Copy link
Contributor

slava77 commented Jan 17, 2015

.. this one is a problematic PR:

fatal: Couldn't find remote ref refs/pull/7192/head

Chris, you have most of them. Is it a special way you submit PRs?

@Dr15Jones
Copy link
Contributor Author

It's a bug in github. It seems to happen if you tell github to create a pull request within a very short time of uploading the commits to github. I hadn't seen the problem in months so I thought they had fixed the race condition. Guess I'll have to go back to waiting 5 minutes or so before creating a pull request.

@Dr15Jones
Copy link
Contributor Author

@ktf any ideas?

@slava77
Copy link
Contributor

slava77 commented Jan 19, 2015

+1

for #7192 213f34e
tested in CMSSW_7_4_0_pre3 /test area sign489/
no changes as expected

local test requires a merge from Dr15Jones:makeStaticConstInSiPixelTemplateSplit directly instead of the upstream

@cmsbuild
Copy link
Contributor

This pull request is fully signed and it will be integrated in one of the next CMSSW_7_4_X IBs unless changes or unless it breaks tests.

@Dr15Jones
Copy link
Contributor Author

@ktf Jenkins can't get the test to run. I'd bet on Slava's tests being sufficient.

ktf added a commit that referenced this pull request Jan 19, 2015
@ktf ktf merged commit 5a36863 into cms-sw:CMSSW_7_4_X Jan 19, 2015
@ktf
Copy link
Contributor

ktf commented Jan 19, 2015

Yeah... I'll contact github support to see if they have any insight about this.

@Dr15Jones Dr15Jones deleted the makeStaticConstInSiPixelTemplateSplit branch January 23, 2015 16:31
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.

None yet

5 participants