[core] rootcling: avoid UB if ROOTSYS not defined

[ferdymercury]

This Pull request:

Changes or fixes:

std::string(nullptr) is ill-defined. Found out by clang-tidy

Checklist:

• tested changes locally
• updated the docs (if necessary)

[guitargeek]

What made you choose return "" instead of return nullptr here? Not saying that one is more correct than the other, both are better than UB. But depending on how GetIncludeDir() is used, one might be better than the other.

Maybe @pcanal has a preference here? I would have gone with nullptr, because otherwise you might break code like if(!GetIncludeDir()) { throw std::runtime_error("include dir not set"); }.

[ferdymercury]

Good point. I just thought it was better not to propagate the issue on the caller-side.
If someone was calling std::string myIncludeDir = GetIncludeDir(...); then he would have to deal with UB again.
This way, he can just check if myIncludeDir.empty()
rather than:
auto iDir = GetIncludeDir();
if(iDir)
std::string myIncludeDir(iDir);

but yeah, it's matter of taste. I am happy either way.

[guitargeek]

Ok I was taking a look in how this function is used, in only rootcling_impl.cxx actually.

If we patch that file to avoid adding using empty paths, we need to patch it like this (assuming nullptr returns):

@@ -4218,8 +4218,10 @@ int RootClingMain(int argc,
       clingArgs.push_back(FullWDiag);
    }

-   std::string includeDir = llvm::sys::path::convert_to_slash(gDriverConfig->fTROOT__GetIncludeDir());
-   clingArgs.push_back(std::string("-I") + includeDir);
+   const char *includeDir = gDriverConfig->fTROOT__GetIncludeDir();
+   if (includeDir) {
+      clingArgs.push_back("-I" + llvm::sys::path::convert_to_slash(includeDir));
+   }

    std::vector<std::string> pcmArgs;
    for (size_t parg = 0, n = clingArgs.size(); parg < n; ++parg) {
@@ -4300,9 +4302,9 @@ int RootClingMain(int argc,
       for (const std::string &modulemap : gOptModuleMapFiles)
          clingArgsInterpreter.push_back("-fmodule-map-file=" + modulemap);

-      clingArgsInterpreter.push_back("-fmodule-map-file=" +
-                                     std::string(gDriverConfig->fTROOT__GetIncludeDir()) +
-                                     "/ROOT.modulemap");
+      if (includeDir) {
+         clingArgsInterpreter.push_back("-fmodule-map-file=" + std::string(includeDir) + "/ROOT.modulemap");
+      }
       std::string ModuleMapCWD = ROOT::FoundationUtils::GetCurrentDir() + "/module.modulemap";
       if (llvm::sys::fs::exists(ModuleMapCWD))
          clingArgsInterpreter.push_back("-fmodule-map-file=" + ModuleMapCWD);

Maybe we can do this in this PR too? If you do the return "" variant that's also fine, but then you need to use strlen() or something to check the length of the string. But I think return nullptr is simpler here.

Whatever you choose, can we also improve the implementation in rootcling_stage1.cxx maybe along the lines I suggested?

[ferdymercury]

Thanks for the review! Sure, changes applied.
Should a Warning or throw be also issued ?
Side note: the implementation logic of GetRootsys in FoundationUtils.cxx is slightly different, with a fallback value.

[github-actions]

Test Results

    17 files      17 suites   4d 15h 18m 36s ⏱️
 2 692 tests  2 691 ✅ 0 💤 1 ❌
45 095 runs  45 094 ✅ 0 💤 1 ❌

For more details on these failures, see this check.

Results for commit 1d39b75.

♻️ This comment has been updated with latest results.