Don't forward declare CaloTower in CaloTowerFwd.h #18805
Conversation
|
Part of the work going on regarding issue #15248 |
|
A new Pull Request was created by @Teemperor (Raphael Isemann) for master. It involves the following packages: DataFormats/CaloTowers @perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
@cmsbuild please test |
|
The tests are being triggered in jenkins. |
|
+1 The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic: |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
should we instead fix the Ref or remove typedefs that can not be forward-declared from Fwd files? |
|
@davidlange6 @Dr15Jones |
|
There is nothing to 'fix' in |
|
On 5/22/17 7:29 AM, Chris Jones wrote:
There is nothing to 'fix' in |Ref|, it is doing what it needs to do. I
think the problem is the name of the file itself. The file was never
used for 'forwarding', instead it defines a set of common typedefs. So
maybe |CaloTowerDefs.h|?
It seems like the easiest option is to open the scope of "Fwd" to be
just for forward declarations and move on with this and similar PRs.
Migration of file names from Fwd to Defs sounds a bit disruptive.
…
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#18805 (comment)>, or
mute the thread
<https://github.com/notifications/unsubscribe-auth/AEdcbm_eWM93rPKq2KwF4ys9wJeMlS__ks5r8Zu7gaJpZM4Nd9Gl>.
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
pending-signatures
reconstruction-pending
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar |
|
Comparison job queued. |
|
Comparison is ready Comparison Summary:
|
|
+1 |
|
+1 |
As discussed in PR cms-sw#18805, the *Fwd.h files aren't working with forward declarations (due to `Ref` needing a definition). This PR brings the same change to the HcalRecHitFwd.h header.
As discussed in PR cms-sw#18805, the *Fwd.h files aren't working with forward declarations (due to `Ref` needing a definition). This PR brings the same change to the HcalRecHitFwd.h header.
As discussed in PR cms-sw#18805, the Fwd.h headers need a definition of the specific class they wrap. We do the same as we did for this PR by renaming th efile to *Defs.h and actually including HGCFETriggerDigi.
As discussed in PR cms-sw#18805, the Fwd.h headers need a definition of the specific class they wrap. We do the same as we did for this PR by renaming th efile to *Defs.h and actually including HGCFETriggerDigi.
As discussed in PR cms-sw#18805, the Fwd.h headers need a definition of the specific class they wrap. We do the same as we did for this PR by renaming th efile to *Defs.h and actually including HGCFETriggerDigi.
As discussed in PR cms-sw#18805, the Fwd.h headers need a definition of the specific class they wrap. We do the same as we did for this PR by renaming th efile to *Defs.h and actually including HGCFETriggerDigi.
As discussed in PR cms-sw#18805, the Fwd.h headers need a definition of the specific class they wrap. We do the same as we did for this PR by renaming th efile to *Defs.h and actually including HGCFETriggerDigi.
This header won't compile with just a forward declaration of
CaloTower, because the Ref and RefVector types we declare will check the
ValueTrait of it's containing class, which is in this case CaloTower.
ValueTrait needs to have the definition of CaloTower to work, but
we only have a forward declaration in this header available.
This leads to undefined behavior, so we should replace this forward
declaration with an actual include.
People seem to anyway always include CaloTower too to
avoid this cryptic error, so I don't think this will hit compilation
times that much.