search

cms-sw / cmssw

CMS Offline Software
http://cms-sw.github.io/
Apache License 2.0
1.08k stars 4.3k forks source link

pragma one vs include guards statement #22754

Open fabiocos opened 6 years ago

fabiocos commented 6 years ago

In the review of #22409 a discussion has started about the possible use of "#pragma once" statements instead of the usual include guards that are the default so far in CMSSW, see https://github.com/cms-sw/cmssw/pull/22409/files#diff-b06786b3e691cb0800930a92ec62ca64 .

The integration of the PR is waiting for a discussion and decision about this choice, although the content of the PR itself does not seem to need the "#pragma once" statement, and could move forward without it in case.

At present the "#pragma once" statement seems to be used within CMSSW only in the CondFormat/Serialization package, see https://cmssdt.cern.ch/lxr/search?%21v=CMSSW_10_1_X_2018-03-26-2300&_filestring=&_string=%23pragma+once

Before expanding its use without a clear strategy, just on the basis of each developer's choice, it would be useful that the Core software group provide some advice about:

1) keep include guards only; 2) progressively migrate to "#pragma once"; 3) a free choice approach.

cmsbuild commented 6 years ago

A new Issue was created by @fabiocos Fabio Cossutti.

@davidlange6, @Dr15Jones, @smuzaffar, @fabiocos can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

fabiocos commented 6 years ago

assign core

cmsbuild commented 6 years ago

New categories assigned: core

@Dr15Jones,@smuzaffar you have been requested to review this Pull request/Issue and eventually sign? Thanks

fabiocos commented 6 years ago

@VinInn @fwyzard @Dr15Jones @davidlange6 @slava77 I think it would be useful to have a discussion about this at next Core software meeting, not only to move forward with #22409 (that is not crucially dependent on this choice I believe) but as a general strategy to follow for the future

fwyzard commented 6 years ago

The integration of the PR is waiting for a discussion and decision about this choice, although the content of the PR itself does not seem to need the "#pragma once" statement, and could move forward without it in case.

22409 is three weeks old, fully signed since two weeks, and at no point it was stated that a discussion about #pragma once was needed before it could be integrated.

VinInn commented 6 years ago

My understanding is that the pragma is irrelevant as the compiler is able to identify full file guards and optimize following access

Sent from my iPhone

On Mar 27, 2018, at 1:55 PM, Andrea Bocci notifications@github.com wrote:

The integration of the PR is waiting for a discussion and decision about this choice, although the content of the PR itself does not seem to need the "#pragma once" statement, and could move forward without it in case.

22409 is three weeks old, fully singed since two weeks, and at no point it was stated that a discussion about #pragma once was needed before it could be integrated.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

VinInn commented 6 years ago

let's try not to replicate https://github.com/RobotLocomotion/drake/issues/2104

fwyzard commented 6 years ago

I honestly don't care what choice we pick, as long as we do take a decision and apply it globally.

davidlange6 commented 6 years ago

Cmssw is basically in such a state now...no? Thus my question.

On 29 Mar 2018, at 14:13, Andrea Bocci notifications@github.com<mailto:notifications@github.com> wrote:

I honestly don't care what choice we pick, as long as we do take a decision and apply it globally.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/cms-sw/cmssw/issues/22754#issuecomment-377215774, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEzyw14DU_lNsHgU_il7D9OWiCHdyCJYks5tjM-zgaJpZM4S8wx-.

fwyzard commented 6 years ago

Thus my question.

@davidlange6 ahem, sorry, but what is your question ?

slava77 commented 6 years ago

On 3/29/18 4:43 AM, Vincenzo Innocente wrote:

let's try not to replicate RobotLocomotion/drake#2104 https://github.com/RobotLocomotion/drake/issues/2104

https://google.github.io/styleguide/cppguide.html#The__define_Guard google style is still ifndef/define and for the Widnows code it explicitly forbids using pragma once.

Chrome https://chromium.googlesource.com/chromium/src/+/master/styleguide/c++/c++.md

Use standard #include guards in all header files (see the Google style guide sections on these for the naming convention). Do not use #pragma once; historically it was not supported on all platforms, and it does not seem to outperform #include guards even on platforms which do support it.

davidlange6 commented 6 years ago

indeed was long ago- and looks like I instead phrased it as a statement:

"seems one wants #pragma once or include guards and not both.To start a migration towards #pragma might be best done with some discussion on the topic. [looks like its easy to find some interesting ones on google]"

ie, don't have two solutions. If the new thing is better lets agree that it is and move with a migration plan that is likely to converge in a finite time..

On Mar 29, 2018, at 2:33 PM, Andrea Bocci notifications@github.com wrote:

Thus my question.

@davidlange6 ahem, sorry, but what is your question ?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

fwyzard commented 6 years ago

Sound good to me.

I'd let somebody else decide if there's anything to gain by moving to '#pragma once' .

Then, I can volunteer some time to migrate packages to it - or remove it and improve the '#ifndef' - if the changes can be merged bypassing the usual signatures.

.A

VinInn commented 6 years ago

I suspect this: https://sft.its.cern.ch/jira/browse/ROOT-7282 will close the issue for a "long" while

davidlange6 commented 6 years ago

Seems so...

On Mar 30, 2018, at 10:35 AM, Vincenzo Innocente notifications@github.com wrote:

I suspect this: https://sft.its.cern.ch/jira/browse/ROOT-7282 will close the issue for a "long" while

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

fwyzard commented 6 years ago

What is an inline header ?

.A

VinInn commented 6 years ago

in reality

git cms-addpkg DataFormats/TrackingRecHit
   133  10:56   git cms-addpkg DataFormats/TrackerRecHit2D
   134  10:56   sed -i '1i #pragma once' DataFormats/Tra*/interface/*.h
   136  10:57   scram b -j 8

does not seem to produce any warning/error of course the guards are still there...

VinInn commented 6 years ago

guards removed: still no warning/error from root-dict generation

VinInn commented 6 years ago

of course somebody did it already at least once https://github.com/marcusmueller/include-guard-convert/blob/master/include-guard-convert.py

davidlange6 commented 6 years ago

The fwk does swallow errors/warnings from root. I guess what matters is when a header gets included more than once for some data format parsing?

On 30 Mar 2018, at 12:16, Vincenzo Innocente notifications@github.com<mailto:notifications@github.com> wrote:

guards removed: still no warning/error from root-dict generation

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://github.com/cms-sw/cmssw/issues/22754#issuecomment-377495765, or mute the threadhttps://github.com/notifications/unsubscribe-auth/AEzyw29HUWufqt8urEkcqbkCC1eWqAa8ks5tjgX0gaJpZM4S8wx-.

VinInn commented 6 years ago

w/o pragma once I do get compiler errors such as 

/tmp/innocent/CMSSW_10_2_X_2018-03-29-2300/src/DataFormats/TrackingRecHit/interface/TrackingRecHit.h:23:7: error: redefinition of 'TrackingRecHit'
class TrackingRecHit {
      ^
/tmp/innocent/CMSSW_10_2_X_2018-03-29-2300/src/DataFormats/TrackingRecHit/interface/TrackingRecHit.h:23:7: note: previous definition is here
class TrackingRecHit {
davidlange6 commented 6 years ago

Interesting.. anyway, maybe its worth a discussion of if this change is desirable and if so to sort out the root issue with their experts

On Mar 30, 2018, at 12:25 PM, Vincenzo Innocente notifications@github.com wrote:

w/o pragma once I do get compiler errors such as

/tmp/innocent/CMSSW_10_2_X_2018-03-29-2300/src/DataFormats/TrackingRecHit/interface/TrackingRecHit.h:23:7: error: redefinition of 'TrackingRecHit' class TrackingRecHit { ^ /tmp/innocent/CMSSW_10_2_X_2018-03-29-2300/src/DataFormats/TrackingRecHit/interface/TrackingRecHit.h:23:7: note: previous definition is here class TrackingRecHit {

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

VinInn commented 6 years ago

I even did

root -b
root [0] #include "DataFormats/TrackerRecHit2D/interface/SiStripRecHit1DCollection.h"

and the only errors/warnings I get are about CMS_THREAD_SAFE

In file included from /cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc700/cms/cmssw-patch/CMSSW_10_2_X_2018-03-29-2300/src/FWCore/MessageLogger/interface/MessageLogger.h:136:
/cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc700/cms/cmssw-patch/CMSSW_10_2_X_2018-03-29-2300/src/FWCore/MessageLogger/interface/MessageDrop.h:110:3: warning: unknown attribute 'thread_safe' ignored [-Wunknown-attributes]
  CMS_THREAD_SAFE static std::string jobMode;                                   // change log 6
  ^
/cvmfs/cms-ib.cern.ch/week1/slc7_amd64_gcc700/cms/cmssw-patch/CMSSW_10_2_X_2018-03-29-2300/src/FWCore/Utilities/interface/thread_safety_macros.h:4:27: note: expanded from macro 'CMS_THREAD_SAFE'
#define CMS_THREAD_SAFE [[cms::thread_safe]]
                          ^
fabiocos commented 6 years ago

@Dr15Jones @VinInn @davidlange6 @fwyzard @slava77 I asked Chris to have a discussion about this at the first Core Software meeting so as to come to an agreement about how to move forward

fabiocos commented 6 years ago

@Dr15Jones @VinInn @davidlange6 @fwyzard @slava77 after the discussion at the Core Software meeting today today there was consensus to keep the use of "#pragma once" on hold for the time being, resuming the issue for the code changes planned for the Long Shutdown.

@fwyzard could you please remove the statement from #22409 so as we move forward with its integration without further delays? The PR has been approved already, so no need to go through another round of approval just for this change

fwyzard commented 6 years ago

@fabiocos I see that @VinInn already made the change.

By the way, the next time you see something as a blocker for merging a PR, could you comment in the PR itself requesting the changes - so it doesn't sit unmerged for one month ?

fabiocos commented 6 years ago

@fwyzard unfortunately this overlapped with several other issues, and my comment was not submitted earlier, sorry for the delay

As discussed at the ORP, I keep this issue open for future memory, as we agreed to possibly come back to this point for LS2.

fabiocos commented 3 years ago

@makortel there was a recent discussion in Mattermost about this, not sure whether keeping this issue open is still meaningful

makortel commented 3 years ago

The mattermost discussion was somewhat inconclusive, so I'd keep the issue open.