Skip to content

[RVec] Forward declare VDT functions in RVec.hxx header #22927

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.

Sign up for GitHub

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

Open
guitargeek wants to merge 1 commit into
base: master
Choose a base branch
from
+32 −2
Open

[RVec] Forward declare VDT functions in RVec.hxx header #22927

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 23 additions & 2 deletions math/vecops/inc/ROOT/RVec.hxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,7 +47,28 @@
#include <vector>

#ifdef R__HAS_VDT
#include <vdt/vdtMath.h>
// Forward declarations of the vdt functions used by the fast_* helpers below.
// We deliberately avoid including <vdt/vdtMath.h> from this public header so
// that we don't rely on this header always being in the include path.
namespace vdt {
inline double fast_exp(double);
inline double fast_log(double);
inline double fast_sin(double);
inline double fast_cos(double);
inline double fast_tan(double);
inline double fast_asin(double);
inline double fast_acos(double);
inline double fast_atan(double);

inline float fast_expf(float);
inline float fast_logf(float);
inline float fast_sinf(float);
inline float fast_cosf(float);
inline float fast_tanf(float);
inline float fast_asinf(float);
inline float fast_acosf(float);
inline float fast_atanf(float);

@vgvassilev vgvassilev Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Should that go in vdt itself as vdt_fwd.h?

@guitargeek guitargeek Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I don't think it's worth it, these forward declaration headers make more sense if the types are complicated and expected to change sometimes (e.g. template types where template arguments are expected to be added).

The VDT function signatures have not changed in 14 years though, so I wouldn't consider a forward declaration header necessary.

@guitargeek guitargeek Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But I think that should be up to the maintainer of vdt, @dpiparo

@vgvassilev vgvassilev Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, but that section can go out of sync if upstream changes. Logistically putting it in the vdt library is the right layering approach afaict.

@guitargeek guitargeek Jul 27, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

But then it doesn't solve the problem anymore: the whole point of this change is to make RVec.hxx work if vdt is not in the include path, for most common case where you don't need the fast_* functions. Like this, it also doesn't end up in the modules.

In the case where you need VDT, you can simply include it yourself. This follows the "only pay for what you use" model that we're generally trying to follow now.

@vgvassilev vgvassilev Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Yes, that should work as you designed it even if the header is in vdt, no? In there if your forward declaration header is standalone we won’t include it as part of the modules setup, either. If you strongly feel that this your approach is better I am fine with it. Just looks like a layering and invalidation problem that we might have. Probably vdt does not change a lot and the benefit is marginal…

@vgvassilev vgvassilev Jul 27, 2026

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I realize that I do not understand the whole idiom. Eg: #define RVEC_VDT_UNARY_FUNCTION(F) RVEC_UNARY_FUNCTION(F, vdt::F). Why do we carry all these in the public header file? If they are implementation defined we might be able to put that logic in a local header file.

} // namespace vdt
#endif


Expand Down Expand Up @@ -1411,7 +1432,7 @@ RVecs as arguments.
- erf, erfc
- lgamma, tgamma

If the VDT library is available, the following functions can be invoked. Internally the calculations
If the VDT library is available and the `<vdt/vdtMath.h>` header included, the following functions can be invoked. Internally the calculations
are vectorized:
- fast_expf, fast_logf, fast_sinf, fast_cosf, fast_tanf, fast_asinf, fast_acosf, fast_atanf
- fast_exp, fast_log, fast_sin, fast_cos, fast_tan, fast_asin, fast_acos, fast_atan
Expand Down
5 changes: 5 additions & 0 deletions math/vecops/src/RVec.cxx
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,11 @@
*************************************************************************/

#include "ROOT/RVec.hxx"

#ifdef R__HAS_VDT
#include <vdt/vdtMath.h>
#endif

using namespace ROOT::VecOps;

// Check that no bytes are wasted and everything is well-aligned.
Expand Down
4 changes: 4 additions & 0 deletions tutorials/math/vecops/vo002_VectorCalculations.C
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,10 @@
/// \date May 2018
/// \author Danilo Piparo

#ifdef R__HAS_VDT
#include <vdt/vdtMath.h>
#endif

void vo002_VectorCalculations()
{

Expand Down
Loading