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

cpp11migrate --addoverride #332

Merged
merged 2 commits into from Aug 21, 2013
Merged

cpp11migrate --addoverride #332

merged 2 commits into from Aug 21, 2013

Conversation

gartung
Copy link
Member

@gartung gartung commented Aug 14, 2013

All override keywords added by the clang extras utility cpp11-migrate --add-override.

@gartung
Copy link
Member Author

gartung commented Aug 14, 2013

Splitting into one branch per package.

@gartung gartung closed this Aug 14, 2013
@gartung gartung deleted the override branch August 14, 2013 20:14
@gartung gartung restored the override branch August 15, 2013 13:59
@gartung gartung reopened this Aug 15, 2013
@gartung
Copy link
Member Author

gartung commented Aug 15, 2013

Everything on one branch.

@ktf
Copy link
Contributor

ktf commented Aug 15, 2013

This one does not compile because:

Fireworks/Core/src/FWEveDigitSetScalableMarker.cc

is included in:

Fireworks/Core/src/classes.h

and genreflex chokes on it. Can you please fix it?

@alja is there any way we can avoid including a cc file in classes.h?

@ghost ghost assigned gartung Aug 15, 2013
@alja
Copy link
Contributor

alja commented Aug 15, 2013

I will split the code into header and source file if this helps to solve the problem.

@gartung
Copy link
Member Author

gartung commented Aug 15, 2013

Backed out change to Fireworks/Core/src/FWEveDigitSetScalableMarker.cc

@Dr15Jones
Copy link
Contributor

ROOT's c++ parser can't handle the C++11 keyword 'override'. So the only way I can see ti fix this is to do something like the following in the header definition

#define OVERRIDE_KEYWORD
#if !defined(__CINT__) && !defined(__MAKECINT__)
#undef OVERRIDE_KEYWORD
#define OVERRIDE_KEYWORD override
#endif

and then replace all occurences of override with OVERRIDE_KEYWORD in that header.

@wmtan
Copy link
Contributor

wmtan commented Aug 15, 2013

I have been using GCCXML, rather than CINT and MAKECINT. That will work, and is more consistent with use elsewhere.

@alja
Copy link
Contributor

alja commented Aug 15, 2013

Thanks for the workaround. Will split the code into header and source anyway. It was my laziness I did not do this at the first place.

@ktf
Copy link
Contributor

ktf commented Aug 16, 2013

@nclopezo can you test?

@ghost ghost assigned nclopezo Aug 16, 2013
@nclopezo
Copy link
Contributor

Hi,

I took this changes on top of CMSSW_7_0_X_2013-08-16-0200, The compilation fails because there is a extra override keyword in the file:

/build/dmendezl/CMSSW_7_0_X_2013-08-16-0200/src/SimGeneral/MixingModule/plugins/HiMixingModule.cc

in line 119

@ktf
Copy link
Contributor

ktf commented Aug 16, 2013

Complete error, please.

@nclopezo
Copy link
Contributor

Hi Giulio,

this is the error message:

/build/dmendezl/CMSSW_7_0_X_2013-08-16-0200/src/SimGeneral/MixingModule/plugins/HiMixingModule.cc:119:9: error: 'override' does not name a type

if you look at the file you can see the extra "override" keyword. Maybe given the amount of files github is not letting me make a comment on the diff.

@ktf
Copy link
Contributor

ktf commented Aug 16, 2013

You are right, it does look like there is an extra override. @gartung can you
look into this?

@gartung
Copy link
Member Author

gartung commented Aug 16, 2013

On 8/16/2013 7:44 AM, Giulio Eulisse wrote:

You are right, it does look like there is an extra override.
@gartung can you
look into this?


Reply to this email directly or view it on GitHub
#332 (comment).

Sorry I missed that in my build log:

/storage/local/data1/gartung/CMSSW_7_0_0_pre1/src/SimGeneral/MixingModule/plugins/HiMixingModule.cc:119:9:
error: 'override' does not name a type

@ktf
Copy link
Contributor

ktf commented Aug 16, 2013

Thanks. @nclopezo can you give it another try when you have time? Thanks.

@cmsbuild
Copy link
Contributor

The following categories have been signed by @fwyzard: HLT

@cms-git-hlt

@cmsbuild
Copy link
Contributor

The following categories have been signed by andreasp (a.k.a. @apfeiffer1 on GitHub): Database

@cms-git-db

2 similar comments
@cmsbuild
Copy link
Contributor

The following categories have been signed by andreasp (a.k.a. @apfeiffer1 on GitHub): Database

@cms-git-db

@cmsbuild
Copy link
Contributor

The following categories have been signed by andreasp (a.k.a. @apfeiffer1 on GitHub): Database

@cms-git-db

@cmsbuild
Copy link
Contributor

The following categories have been signed by ciulli (a.k.a. @vciulli on GitHub): Generators

@cms-git-generators

@cmsbuild
Copy link
Contributor

The following categories have been signed by @demattia: Calibration and Alignment

@cms-git-alca

@cmsbuild
Copy link
Contributor

The following categories have been signed by speer (a.k.a. @thspeer on GitHub): Reconstruction

@cms-git-reconstruction

@nclopezo
Copy link
Contributor

Hi,

I took this changes on top of CMSSW_7_0_X_2013-08-19-0200, I ran the unit tests and RelVals. All tests passed.

@cmsbuild
Copy link
Contributor

The following categories have been signed by chrjones (a.k.a. @Dr15Jones on GitHub): Visualization, Geometry, Core

@cms-git-visualization, @cms-git-core, @cms-git-geometry

@cmsbuild
Copy link
Contributor

The following categories have been signed by @vadler: Analysis

@cms-git-analysis

ktf added a commit that referenced this pull request Aug 21, 2013
Move FWEveDigitSetScalableMarker delcaration in header file. Related to #332
@gartung
Copy link
Member Author

gartung commented Aug 21, 2013

Rebased to current cmssw/CMSSW_7_0_X

@ktf
Copy link
Contributor

ktf commented Aug 21, 2013

I built and run the tests.

https://cmssdt.cern.ch/jenkins/job/Pull-Request-Integration/ARCHITECTURE=slc5_amd64_gcc472/204/console

since this is really technical changes, I'll bypass.

ktf added a commit that referenced this pull request Aug 21, 2013
cpp11migrate --addoverride
@ktf ktf merged commit cb3164c into cms-sw:CMSSW_7_0_X Aug 21, 2013
@gartung gartung deleted the override branch August 22, 2013 14:25
makortel pushed a commit to makortel/cmssw that referenced this pull request Apr 22, 2015
PR Tests: test clang compilation and improve results reporting script
gpetruc added a commit to gpetruc/cmssw that referenced this pull request May 7, 2015
…ffInHeppy

Heppy: new features in Photon object/analyzer
fwyzard added a commit to makortel/cmssw that referenced this pull request Apr 23, 2019
slava77 pushed a commit to slava77/cmssw that referenced this pull request Oct 9, 2021
PR cms-sw#327 (solving existing conflicts) + dead modules option
FHead pushed a commit to FHead/cmssw that referenced this pull request Nov 5, 2021
…tion. Note also that the result with or without cone exclusion enabled are just scaled versions of each other. (cms-sw#332)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment