Skip to content

Conversation

@eguiraud
Copy link
Contributor

@eguiraud eguiraud commented May 13, 2017

Second and last PR to resolve ROOT-8780 ("Less namespace verbosity and more uniform namings for TDataFrame").

@phsft-bot
Copy link

Starting build on gcc49/centos7, native/mac1012, gcc49/slc6, gcc62/slc6, native/ubuntu14 with CMake flags -Dvc=OFF -Dimt=ON -Dccache=ON

@phsft-bot
Copy link

Build failed on mac1012/native.
See console output.

@phsft-bot
Copy link

@phsft-bot
Copy link

@phsft-bot
Copy link

@phsft-bot
Copy link

@phsft-bot
Copy link

Starting build on gcc49/centos7, native/mac1012, gcc49/slc6, gcc62/slc6, native/ubuntu14 with CMake flags -Dvc=OFF -Dimt=ON -Dccache=ON

@phsft-bot
Copy link

Build failed on mac1012/native.
See console output.

@phsft-bot
Copy link

@phsft-bot
Copy link

@phsft-bot
Copy link

@phsft-bot
Copy link

@pcanal
Copy link
Member

pcanal commented May 13, 2017

For the header files, I think technically we ought to have the header declaring classes inside a namespace (excluding/ignoring Experimental and possibly Details) in a subdirectory (see include/Math)

Since TDataFrame.hxx (the only user header file If I understood correctly) pull TDataFrame into the ROOT namespace I will keep it in [include]/ROOT but I would move all the other header in ROOT/TDF (and ROOT/Details) to keep things tidier.

@eguiraud
Copy link
Contributor Author

@pcanal the namespaces we use are ROOT::{Detail,Experimental,Internal}::TDF, so I do not totally understand your proposal. Also $ROOTSYS/tree/treeplayer/inc/ROOT/TDF is an awful long pathname, but if that's the rule I will make the necessary changes to comply of course.

@pcanal
Copy link
Member

pcanal commented May 14, 2017

It would be $ROOTSYS//ROOT/TDF/DetailsHeader.hxx and would only be used by our own headers not by the user. (and yes the files in git would in tree/treeplayer/inc/ROOT/TDF)

@dpiparo
Copy link
Member

dpiparo commented May 14, 2017

I am in favour of keeping things tidy. Still I find more informative/maintainable to have names for headers which refer to what they contains rather than a generic "Details".
If we want to change the policy related to paths in general for ROOT it is perhaps worth thinking this through and maybe go for it in November?

@phsft-bot
Copy link

Starting build on gcc49/centos7, native/mac1012, gcc49/slc6, gcc62/slc6, native/ubuntu14 with CMake flags -Dvc=OFF -Dimt=ON -Dccache=ON

@phsft-bot
Copy link

Build failed on mac1012/native.
See console output.

@phsft-bot
Copy link

Starting build on gcc49/centos7, native/mac1012, gcc49/slc6, gcc62/slc6, native/ubuntu14 with CMake flags -Dvc=OFF -Dimt=ON -Dccache=ON

@phsft-bot
Copy link

Build failed on mac1012/native.
See console output.

@eguiraud
Copy link
Contributor Author

clang-format complains about the usual markdown tables longer than 120 lines (I have not changed them).

Test builds look good enough...mac did not finish due to a seemingly unrelated failure, while root_dataframe_functiontraits is fixed by a related PR in roottest.

eguiraud added 4 commits May 15, 2017 15:39
Inside the ns are:
* TCustomColumn[Base] (was TDataFrameBranch[Base])
* TRange (was TDataFrameRange)
* TFilter (was TDataFrameFilter)
* TDataFrameImpl
* BranchNames_t (was in namespace ROOT)
Also rename TDataFrameAction[Base] -> TAction[Base].
Merge Internal::{Operations,TDFV7Utils} into Internal::TDF.
@phsft-bot
Copy link

Starting build on gcc49/centos7, native/mac1012, gcc49/slc6, gcc62/slc6, native/ubuntu14 with CMake flags -Dvc=OFF -Dimt=ON -Dccache=ON

@phsft-bot
Copy link

@phsft-bot
Copy link

@phsft-bot
Copy link

@phsft-bot
Copy link

@phsft-bot
Copy link

@dpiparo dpiparo merged commit a18336b into root-project:master May 15, 2017
@eguiraud eguiraud deleted the tdf_naming branch May 16, 2017 08:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants