Skip to content

Conversation

@ferdymercury
Copy link
Collaborator

This Pull request:

Changes or fixes:

according to my (limited) tests, interpreter still works without needing to prepend std:: on any class, so this doesn't change behavior

Checklist:

  • tested changes locally

@github-actions
Copy link

github-actions bot commented Dec 3, 2024

Test Results

    18 files      18 suites   3d 17h 22m 12s ⏱️
 2 666 tests  2 663 ✅ 0 💤 3 ❌
46 270 runs  46 266 ✅ 0 💤 4 ❌

For more details on these failures, see this check.

Results for commit b848e4c.

♻️ This comment has been updated with latest results.

@ferdymercury ferdymercury marked this pull request as ready for review December 3, 2024 11:54
@ferdymercury ferdymercury requested a review from hahnjo December 3, 2024 13:35
Copy link
Member

@pcanal pcanal left a comment

Choose a reason for hiding this comment

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

We should clarify here where the/an other injection of the using namespace std is done. It may (or may not) be that this injection is the logical place to keep (and remove the other instead).

@ferdymercury
Copy link
Collaborator Author

I think/believe in TClingUtils it's not an injection per se of the whole namespace, but rather that it checks if a class is defined within std. Changing it here is straightforward, in the other place I believe much more complicated

@dpiparo dpiparo closed this Dec 4, 2024
@dpiparo dpiparo reopened this Dec 4, 2024
@pcanal
Copy link
Member

pcanal commented Dec 4, 2024

Indeed. The injection per name is the right direction. Please update the commit log to point to where in TClingUtils this is happening.

Copy link
Member

@hahnjo hahnjo left a comment

Choose a reason for hiding this comment

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

I believe this is far more complicated than just removing the using namespace std, see this PR by Axel from two years ago: #11027

@ferdymercury
Copy link
Collaborator Author

ferdymercury commented Dec 4, 2024

I believe this is far more complicated than just removing the using namespace std, see this PR by Axel from two years ago: #11027

This PR is not aiming at any change in behavior (what Axel tried). It's just "cleaning up the code",. Nothing will change. You will still get the (for some) annoying behavior of having everything within std there in the interpreter. Here in this PR, we just remove a redundant statement.

Note that all tests pass, and the interpreter works as before, which means that this basically like a no-op commit.

The TClingUtils class already takes care of finding if it's a class from the STD, it's irrelevant if you called using namespace std or not.
See e.g. https://github.com/root-project/root/blob/0ca04325af4ccbb4c7ad553b63679c902096feaa/core/clingutils/src/TClingUtils.cxx#L4453
On the other hand, rootcling has its own using namespace std statement, so this change does not affect rootcling either.

The injection of std on a case-by-case basis based on the name is already taken care of in, so this Pre-include statement here is unnecessary or functionally redundant.
In other words, this just a code cleanup that introduces no functional change at all.
interpreter still works without needing to prepend std::
@ferdymercury ferdymercury requested a review from pcanal December 4, 2024 16:39
@ferdymercury ferdymercury changed the title [metacling] remove unnecessary using namespace std since std is autoinjected later in TClingUtils [metacling] remove redundant 'using namespace std' since std is autoinjected later in TClingUtils Dec 4, 2024
@hahnjo
Copy link
Member

hahnjo commented Dec 4, 2024

The explanation is not correct, TClingUtils is not involved in injecting std:: for interpreted code: If you print the code received in cling::Interpreter, you see that it's not qualified. The only reason this still works is that the dictionaries and pcms have using namespace std baked into them, so even after removing the "redundant" declaration in this PR the interpreter gets it as soon as it loads the first pcm, which is basically the first action it does. I think we should not rely on this side effect, the explicit using namespace std that we have right now is better.

@pcanal
Copy link
Member

pcanal commented Dec 4, 2024

gets it as soon as it loads the first pcm,

Humm ... Did we test this PR with -Druntime_cxxmodules=OFF?

@ferdymercury
Copy link
Collaborator Author

gets it as soon as it loads the first pcm,

Humm ... Did we test this PR with -Druntime_cxxmodules=OFF?

See https://github.com/root-project/root/actions/runs/12164244293/job/33925453332?pr=17175

@hahnjo
Copy link
Member

hahnjo commented Dec 4, 2024

gets it as soon as it loads the first pcm,

Humm ... Did we test this PR with -Druntime_cxxmodules=OFF?

Ok, I wasn't 100% precise: with runtime_cxxmodules=OFF (also default on Windows), you get the using declaration via the pch. The point remains: removing the explicit declaration makes us rely on a side effect of automatic header / pcm / pch loading

@ferdymercury
Copy link
Collaborator Author

ferdymercury commented Dec 4, 2024

I think we should not rely on this side effect, the explicit using namespace std that we have right now is better.

I understand. Feel free to close this PR in that case.

I just thought, if at some point, someone tries to restart with #11027, there was one line less to worry about. Speaking about 11027 and potential ways to slowly going away from global std, from my point of view, it would be nice to have something equivalent to this config variable:

# You can set the variable below to 0 to disable the loading of these
# includes at startup. You can set the variable to 1 (default) to load
# only <iostream>, <string> and <DllImport.h>. You can set it to 2 to
# load in addition <vector> and <utility>.  We strongly recommend setting
# the variable to 2 if your scripts include <vector> and you execute
# your scripts multiple times.
Rint.Includes:      1

So, one could have this interpreter behavior:
Rint.NamespaceStd is 2 by default and calls using namespace std;
If you set it to 1, it just calls using std::string, std::cout, std::endl, std::vector, std::cerr, std::cin, std::array;
If you set it to 0, you need to do things correctly as in a C++ program.

In ROOT 6.42 one could set the default to 1, and in ROOT 9 set the default to 0 ;)

This could be independent on dictionary generation, I guess, that could stay as before with full namespace std statement.

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