Skip to content

Addition of Unique Ptr type Open interface with implementation #14017

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

JRS296
Copy link

@JRS296 JRS296 commented Nov 3, 2023

This Pull request:

Addition of Unique Pointer for TFile::Open -> Creation of Interface and implementation for the same

Changes or fixes:

Enables utilization of Unique Pointer for TFile::Open

Checklist:

  • tested changes locally

@JRS296 JRS296 requested a review from pcanal as a code owner November 3, 2023 19:48
@phsft-bot
Copy link

Can one of the admins verify this patch?

@@ -299,6 +299,11 @@ class TFile : public TDirectoryFile {
static TFile *Open(const char *name, Option_t *option = "",
const char *ftitle = "", Int_t compress = ROOT::RCompressionSetting::EDefaults::kUseCompiledDefault,
Int_t netopt = 0);

static std::unique_ptr<TFile> OpenU(const char *name, Option_t *option = "",
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
static std::unique_ptr<TFile> OpenU(const char *name, Option_t *option = "",
static std::unique_ptr<TFile> OpenUniquePtr(const char *name, Option_t *option = "",

Copy link
Member

Choose a reason for hiding this comment

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

We shall have a naming discussion in Mattermost's I/O channel. I promise that nobody will want to spell OpenUniquePtr :-) And I don't want to see Bronch-style Opan either :-)

Copy link
Author

Choose a reason for hiding this comment

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

@Axel-Naumann @pcanal Is it fine if we keep it as OpenU for the time being?

@pcanal
Copy link
Member

pcanal commented Nov 3, 2023

@phsft-bot build

@phsft-bot
Copy link

Starting build on ROOT-performance-centos8-multicore/soversion, ROOT-ubuntu2204/nortcxxmod, ROOT-ubuntu2004/python3, mac12arm/cxx20, windows10/default
How to customize builds

@phsft-bot
Copy link

Build failed on ROOT-ubuntu2004/python3.
Running on root-ubuntu-2004-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

  • [2023-11-03T21:57:54.290Z] FAILED: io/io/CMakeFiles/RIO.dir/src/TFile.cxx.o
  • [2023-11-03T21:57:54.546Z] /usr/include/c++/9/bits/unique_ptr.h:857:30: error: no matching function for call to ‘TFile::TFile(const char**, const char**, const char**, int&, int&)’

@phsft-bot
Copy link

Build failed on ROOT-ubuntu2204/nortcxxmod.
Running on root-ubuntu-2204-1.cern.ch:/home/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

  • [2023-11-03T21:57:03.391Z] /usr/include/c++/11/bits/unique_ptr.h:962:30: error: no matching function for call to ‘TFile::TFile(const char**, const char**, const char**, int&, int&)’

@phsft-bot
Copy link

Build failed on ROOT-performance-centos8-multicore/soversion.
Running on olbdw-01.cern.ch:/data/sftnight/workspace/root-pullrequests-build
See console output.

Errors:

  • [2023-11-03T22:04:50.155Z] /usr/include/c++/8/bits/unique_ptr.h:835:30: error: no matching function for call to ‘TFile::TFile(const char**, const char**, const char**, int&, int&)’

@phsft-bot
Copy link

Build failed on mac12arm/cxx20.
Running on macphsft26.dyndns.cern.ch:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

Errors:

  • [2023-11-03T22:08:52.845Z] FAILED: io/io/CMakeFiles/RIO.dir/src/TFile.cxx.o
  • [2023-11-03T22:08:53.150Z] /Library/Developer/CommandLineTools/SDKs/MacOSX13.1.sdk/usr/include/c++/v1/__memory/unique_ptr.h:728:32: error: no matching constructor for initialization of 'TFile'

@phsft-bot
Copy link

Build failed on windows10/default.
Running on null:C:\build\workspace\root-pullrequests-build
See console output.

Errors:

  • [2023-11-03T22:34:54.792Z] C:\Program Files\Microsoft Visual Studio\2022\Community\VC\Tools\MSVC\14.32.31326\include\memory(3423,35): error C2661: 'TFile::TFile': no overloaded function takes 5 arguments [C:\build\workspace\root-pullrequests-build\build\io\io\RIO.vcxproj]

@JRS296
Copy link
Author

JRS296 commented Nov 5, 2023

I've just got the testing framework ready, and will conduct local tests before further review. (Just read the PR code of conduct, my apologies for creating a PR without the local tests)

@Axel-Naumann Axel-Naumann marked this pull request as draft November 6, 2023 17:23
@JRS296
Copy link
Author

JRS296 commented Nov 8, 2023

@Axel-Naumann I've left a message on the root web channel https://mattermost.web.cern.ch/root/channels/root-io, could you have a look?

@JRS296 JRS296 marked this pull request as ready for review November 9, 2023 05:45
@JRS296
Copy link
Author

JRS296 commented Nov 9, 2023

@pcanal Successfully tested changes locally, ready for review 👍

Copy link

github-actions bot commented Nov 9, 2023

Test Results

       11 files         11 suites   1d 16h 53m 48s ⏱️
  2 481 tests   2 481 ✔️ 0 💤 0
26 220 runs  26 220 ✔️ 0 💤 0

Results for commit a829fc3.

♻️ This comment has been updated with latest results.

JRS296 and others added 2 commits November 10, 2023 00:11
@JRS296 JRS296 marked this pull request as draft November 9, 2023 21:42
@JRS296
Copy link
Author

JRS296 commented Nov 10, 2023

@pcanal I had been running the master branch for tests and builds all this time, and not my patch branch; that's why its been failing the Jekyll build 😅
It should be ready now. My apologies for the repeated reviews.

@JRS296 JRS296 marked this pull request as ready for review November 10, 2023 12:44
@JRS296
Copy link
Author

JRS296 commented Nov 13, 2023

@pcanal I had been running the master branch for tests and builds all this time, and not my patch branch; that's why its been failing the Jekyll build 😅 It should be ready now. My apologies for the repeated reviews.

Can I have the review for this change?

@JRS296
Copy link
Author

JRS296 commented Nov 14, 2023

@pcanal For some reason the clang tools is running on a different branch, and is not addressing the latest commit to this branch.. Everything else seems fine.

Also as a side note, should I rebase all of this to a single commit?

@JRS296
Copy link
Author

JRS296 commented Nov 16, 2023

@pcanal Can this be merged? And must I rebase this to clean up the commit history?

@Axel-Naumann
Copy link
Member

Thanks @JRS296 ! We need to decide how to proceed; the name we choose will become part of ROOT's API and we need to keep it backward compatible from there on. We will come back to you hopefully tonight (@pcanal )

@JRS296
Copy link
Author

JRS296 commented Nov 20, 2023

Thanks @JRS296 ! We need to decide how to proceed; the name we choose will become part of ROOT's API and we need to keep it backward compatible from there on. We will come back to you hopefully tonight (@pcanal )

@Axel-Naumann Any updates on the naming convention to be used? Also, I'd like to know if there is a possibility for me adding some unit test cases for the newly implemented interface, thanks.

@pcanal
Copy link
Member

pcanal commented Nov 21, 2023

We are still discussing the names. The current proposal is:

   using TFilePtr = std::unique_ptr<TFile>;

   /// Open a file with `name` for reading.
   ///
   /// \note: Synchronizes multi-threaded accesses through locks.
   static TFilePtr OpenForRead(std::string_view name, const Options_t &opts = Options_t());

   /// Open an existing file with `name` for reading and writing. If a file with
   /// that name does not exist, an invalid RFilePtr will be returned.
   ///
   /// \note: Synchronizes multi-threaded accesses through locks.
   static TFilePtr OpenForUpdate(std::string_view name, const Options_t &opts = Options_t());

   /// Open a file with `name` for reading and writing. Fail (return an invalid
   /// `RFilePtr`) if a file with this name already exists.
   ///
   /// \note: Synchronizes multi-threaded accesses through locks.
   static TFilePtr Create(std::string_view name, const Options_t &opts = Options_t());

   /// Open a file with `name` for reading and writing. If a file with this name
   /// already exists, delete it and create a new one. Else simply create a new file.
   ///
   /// \note: Synchronizes multi-threaded accesses through locks.
   static TFilePtr Recreate(std::string_view name, const Options_t &opts = Options_t());

In the meantime, you should indeed start getting familiar with the testing infrastructure. See io/io/test for some examples.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants