C++17 fixes

[davidlt]

I have moved CMSSW to the latest GCC 7.1.1 and to the tip of 6.10 patches branch. These are the fixes I have applied on top of that branch for CMS.

[phsft-bot]

Can one of the admins verify this patch?

[pcanal]

Don't we need to match clang value?

[davidlt]

There is no real need. The compiler will not care as it's not using this value. Clang 3.9 which is embedded in ROOT already supported majority of C++17 features. Changing __cplusplus to a proper value does not mean that C++17 is out experimental land. This is basically for people who use __cplusplus to figure out what C++ features they want to use.

[pcanal]

Isn't data() not guaranteed to be null terminated? (which WriteObjectAny requires)?

[davidlt]

You are right. Starting C++11 std::string .data() and .c_str() both provide null-terminated result, but the same does not apply to std::string_view. The example I am looking even shows that one needs to create a temporary string and then call .data().

[davidlt]

Could you point me to the comment in the code which states that it's required? At first look I don't see it explicitly mentioned in TBufferFile::WriteObjectClass or TBufferFile::WriteObjectAny and I didn't see any breakage in CMSSW.

[davidlt]

Ops, it should be TDirectoryFile::WriteObjectAny, but same story. No comments mention that it should be null-terminated. Even provided example is not null-terminated.

[pcanal]

Starting C++11 std::string .data() and .c_str() both provide null-terminated result

I somehow did not catch that. Thanks!

Could you point me to the comment in the code which states that it's required?

Humm ... WriteObjectAny takes a const char* and use it as null terminated string, I am not sure why there would be a 'special' comment in this routine to make that explicit ...

It is clear that std::string_view::data is not appropriate here, as this returns the first characters of the view, which could for example be a sub-part of a larger string.

[davidlt]

Weekend already. I was looking at completely different argument :)

[pcanal]

Don't we need a null terminated string here?
Alternatively, what about adding a string_view overload of ::TFile::SetCacheFileDir ?

[davidlt]

Yeah, we could do that.

[davidlt]

At the end I didn't like idea of overload. It would cause more lines to be changed at this point and wouldn't work with TString (ambiguity). Let's do more minimal change for now.

[pcanal]

Actually, we don't need an overload just a replacement as a string_view can be constructed implicitly from a const char*. I uploaded the change in signature of TFile::SetCacheFileDir.

[davidlt]

@pcanal what else do we need to do for this PR? I probably need to change lines to TFile::SetCacheFileDir to use your new overload.

[pcanal]

I probably need to change lines

Yes, please :)

[pcanal]

I merged the non-string_view changes in the master and 6.10.

Reviewing the string_view changes made us rethink whether string_view ought to be used here ...

[davidlt]

I am looking into this PR, but ROOT doesn't compile anymore in C++17 after LLVM update (I think, I found why -- testing).

The patch to change cplusplus is no more needed as exact same thing is already in newer integrated LLVM.

[Axel-Naumann]

Thanks, David!

[davidlt]

Just to summarize, the following still needs to be resolved:

diff --git a/io/io/v7/inc/ROOT/TFile.hxx b/io/io/v7/inc/ROOT/TFile.hxx
index dd62024076..020ea83789 100644
--- a/io/io/v7/inc/ROOT/TFile.hxx
+++ b/io/io/v7/inc/ROOT/TFile.hxx
@@ -160,7 +160,7 @@ public:

   /// Write an object that is already lifetime managed by this TFileImplBase.
   void Write(std::string_view name) {
-    auto dep = Find(name.to_string());
+    auto dep = Find(std::string(name.data(), name.size()));
     WriteMemoryWithType(name, dep.GetPointer().get(), dep.GetType());
   }

diff --git a/io/io/v7/src/TFile.cxx b/io/io/v7/src/TFile.cxx
index 4d75333c46..1e01456327 100644
--- a/io/io/v7/src/TFile.cxx
+++ b/io/io/v7/src/TFile.cxx
@@ -97,7 +97,7 @@ public:
   }

   void WriteMemoryWithType(std::string_view name, const void* address, TClass* cl) final {
-    fOldFile->WriteObjectAny(address, cl, name.to_string().c_str());
+    fOldFile->WriteObjectAny(address, cl, std::string(name.data(), name.size()).c_str());
   }
 };
 }
@@ -144,7 +144,7 @@ OpenV6TFile(std::string_view name, const char* mode,
     }
   } setCacheDirRAII(opts.fCachedRead);

-  auto v6storage = std::make_unique<TV6Storage>(name.to_string(), GetV6TFileOpts(mode, opts));
+  auto v6storage = std::make_unique<TV6Storage>(std::string(name.data(), name.size()), GetV6TFileOpts(mode, opts));

   using namespace ROOT::Experimental::Internal;
   return std::unique_ptr<TFileStorageInterface>{std::move(v6storage)};
@@ -191,7 +191,7 @@ std::string ROOT::Experimental::TFile::SetCacheDir(std::string_view path) {
   std::lock_guard<std::mutex> lock(GetCacheDirMutex());

   std::string ret = ::TFile::GetCacheFileDir();
-  ::TFile::SetCacheFileDir(path.to_string().c_str());
+  ::TFile::SetCacheFileDir(path);
   return ret;
 }

[amadio]

@davidlt Could you please rebase and push so that we can test against latest master? Thanks!

[davidlt]

@amadio to my understanding there is no resolution how to resolve it. See a comment from @pcanal #591 (comment)

Thus currently this PR acts as a reminder/todo.

[amadio]

@davidlt I have changes to remove usage of string view from ROOT7's TFile in C++17 mode, but I wanted to check the status of this PR before merging them in. If you could remove your changes that were merged by Philippe and keep the rest, I can take a look and cherry-picking your fixes into master as needed.

[davidlt]

Based on this PR the only thing what's left is TFile changes (unless new breakage was introduced). The rest is already in to my knowledge. Thus you if you merge TFile changes you can close this one.

[amadio]

Alright, I guess I will merge my changes and close this then. Thanks!