root-project / root

The official repository for ROOT: analyzing, storing and visualizing big data, scientifically
https://root.cern
Other
2.63k stars 1.25k forks source link

Injecting class span inside the std namespace is asking for problem #13042

Closed RoyBellingan closed 1 year ago

RoyBellingan commented 1 year ago

Check duplicate issues.

Describe the bug

https://github.com/root-project/root/blob/master/core/foundation/inc/ROOT/span.hxx#L153

Manipulating in any way the std namespace should not be done, this is why namespace exists in the first place, to separate things

In this case this will collide with https://en.cppreference.com/w/cpp/container/span

What is the expected behaviour?

If you want / need to use a span adaptor and you can not use c++20 do not inject in the std namespace but use another one, this is why namespace exists!

How to reproduce?

https://github.com/root-project/root/blob/master/core/foundation/inc/ROOT/span.hxx#L153 is violating c++ guidelines

ROOT version

master

How did you install ROOT?

irrelevant

Which operating system are you using?

irrelevant

Additional context

https://github.com/root-project/root/blob/master/core/foundation/inc/ROOT/span.hxx#L153 is violating c++ guidelines

pcanal commented 1 year ago

core/foundation/inc/ROOT/span.hxx should never be used in the same build as the official span. It is literally meants to be a bridge from older C++ standard (where it is not available) to the newer standard (where it is available).

Which standard did you build ROOT with? Which standard did you build your code with?

RoyBellingan commented 1 year ago

I am using C++23, changed now to 17.

I do not have control on who is including what let me dig for you, I will put back 23 and debug more

RoyBellingan commented 1 year ago
In file included from /usr/include/root/TCollection.h:33,
                 from /usr/include/root/TSeqCollection.h:25,
                 from /usr/include/root/TList.h:25,
                 from /usr/include/root/TQObject.h:40,
                 from /usr/include/root/TSysEvtHandler.h:25,
                 from /usr/include/root/TTimer.h:45,
                 from /usr/include/root/TSystem.h:36,
                 from /home/roy/geant/cresta/src/ROOTHeaders.hh:12,
                 from /home/roy/geant/cresta/src/core/Analysis.hh:4,
                 from /home/roy/geant/cresta/src/CorePackage.hh:5,
                 from /home/roy/geant/cresta/src/construction/cresta/CRESTAWorld.hh:22,
                 from /home/roy/geant/cresta/src/construction/WorldConstruction.cc:21:
/usr/include/root/ROOT/RRangeCast.hxx:186:33: error: template argument 3 is invalid
  186 | RRangeCast<T, false, std::span<U>> RangeStaticCast(U (&arr)[N])
      |                                 ^~
/usr/include/root/ROOT/RRangeCast.hxx: In function ‘int ROOT::RangeStaticCast(U (&)[N])’:
/usr/include/root/ROOT/RRangeCast.hxx:188:16: error: reference to ‘span’ is ambiguous
  188 |    return std::span<U>(arr, arr + N);

I will check if I can remove some of the header getting included

RoyBellingan commented 1 year ago

I think a quick fix could be to just ifdef the whole file, in my case the inclusion is in https://github.com/root-project/root/blob/05c2b9ef3ff35b9023eb7b9e20db412f5f8dd89b/core/cont/inc/TCollection.h#L33

RoyBellingan commented 1 year ago

I see https://github.com/root-project/root/pull/11311

Is missing the R__HAS_STD_SPAN, I would say to avoid more improper reporting, it should be made more clear when those header are imported that this is the intended behaviour.

I would say you should add a

#if __cplusplus >= 202002L

inside those code block if they are enabled due to this mismatched compilation version and report the error

pcanal commented 1 year ago

I am using C++23,

We do not yet support C++23 as we are still using llvm 15 for the interpreter

But even in C++23, the ROOT build should have configured itself to NOT actually use ROOT/span.hxx (i.e. see the beginning of the file):

#if __cplusplus >= 202002
# if __has_include(<span>)
#  include <span>
# endif
#endif

#if !defined(__cpp_lib_span)
pcanal commented 1 year ago

I see https://github.com/root-project/root/pull/11311

This was not merged and does not reflect that actual content of the file. Somehow in your environment the code snippet I copy/pasted is failing.

We need to understand if:

This should be testable with a simple source file containing just:

#include <span>
#include "ROOT/RRangeCast.hxx"

(and another test with just the second one).

RoyBellingan commented 1 year ago

@pcanal thank you for the support! I will now check why this block

#if __cplusplus >= 202002
# if __has_include(<span>)
#  include <span>
# endif
#endif

Is not working.

| With c++ (SUSE Linux) 12.2.1 20230124 [revision 193f7e62815b4089dfaed4c2bd34fd4f10209e27] |

Because I do not have master, I will check if my distro as a current version of root.

Sorry but seeing the std namespace violation "tunnel visioned me" -.-

No opensuse tumbleweed has root until 6.26, I will patch in local, thank you for the support, and sorry for the bug report.