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

Try to hide fwd include in doxygen #8224

Closed
wants to merge 1 commit into from
Closed

Try to hide fwd include in doxygen #8224

wants to merge 1 commit into from

Conversation

couet
Copy link
Member

@couet couet commented May 21, 2021

Try to better fix #8051
In the files Vector2-4-3D.h the definitions in the fwd files are now duplicated,
in order typedef like PtEtaPhiMVector appears defined in Vector4D.h in the ref guide
as it is the file users should include.

@couet couet requested a review from lmoneta May 21, 2021 09:26
@couet couet self-assigned this May 21, 2021
@phsft-bot
Copy link
Collaborator

Starting build on ROOT-debian10-i386/cxx14, ROOT-performance-centos8-multicore/default, ROOT-fedora30/cxx14, ROOT-fedora31/noimt, ROOT-ubuntu16/nortcxxmod, mac1014/python3, mac11.0/cxx17, windows10/cxx14
How to customize builds

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-fedora31/noimt.
Running on root-fedora-31-2.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Failing tests:

@phsft-bot
Copy link
Collaborator

Build failed on ROOT-debian10-i386/cxx14.
Running on pcepsft10.dyndns.cern.ch:/build/workspace/root-pullrequests-build
See console output.

Failing tests:

Copy link
Member

@lmoneta lmoneta left a comment

Choose a reason for hiding this comment

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

I understand the PR now fixes the documentation by having the definition of the typedef's directly in Vector.h
However, we are repeating the code, and I am afraid this could cause a problem.
I do not see really an advantage of this. It will add a link to Vector4d.h that anyway does not contain the definition of the templated class that is in LorentzVector.h
The documentation of the class is directly accessible from the typedef definition.
I would then leave the current status and close this PR

@Axel-Naumann
Copy link
Member

Axel-Naumann commented Jun 21, 2021

I am afraid this could cause a problem.

Which one?

As long as users see the template, #include (not the forward!), and documentation when entering PtEtaPhiMVector in the Doxygen search I'll be happy :-)

@lmoneta
Copy link
Member

lmoneta commented Jun 21, 2021

The problem could be that one changes the template class and the typedef declaration in Vector.h but forgets to modify Vectorfwd.h. If that tupedef's is not tested there is an inconsistency.

We have this now https://root.cern.ch/doc/master/namespaceROOT_1_1Math.html#a39def91bfd150148b1534d8ae665b145

@Axel-Naumann
Copy link
Member

Simply include ...fwd.h in the ...h header and you're guaranteed that they typedefs are consistent.

@lmoneta
Copy link
Member

lmoneta commented Jun 21, 2021

This is exactly what is now in master and this PR is changing this

@couet couet closed this Aug 20, 2021
@couet couet deleted the fwd-include-and-doxygen branch August 20, 2021 09:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Fixed in 6.26/00
Awaiting triage
Development

Successfully merging this pull request may close these issues.

Impossible to understand what to #include for ROOT::Math::PtEtaPhiMVector etc
4 participants