Skip to content

[core] use one instead of two lookups within TClass::Init() #14760

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

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

ferdymercury
Copy link
Collaborator

This Pull request:

Changes or fixes:

Fixes #7123

Checklist:

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

@phsft-bot

This comment was marked as outdated.

@ferdymercury

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

This comment was marked as outdated.

@pcanal

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

1 similar comment
@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

2 similar comments
@phsft-bot

This comment was marked as outdated.

@phsft-bot

This comment was marked as outdated.

@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.

@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.

@phsft-bot
Copy link

Build failed on mac12arm/cxx20.
Running on 194.12.161.128:/Users/sftnight/build/workspace/root-pullrequests-build
See console output.

@ferdymercury ferdymercury requested a review from pcanal February 24, 2024 10:04
@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:

  • [2024-02-24T10:08:15.566Z] FAILED: tmva/sofie/test/CMakeFiles/SofieCompileModels_ROOT.util

@ferdymercury
Copy link
Collaborator Author

Thanks for the review. I reverted to the original behavior as suggested, but the problem is still there.

Maybe the problem is that TClingClassInfo::TClingClassInfo(cling::Interpreter *interp, const Decl* decl) sets the decl found by CheckClassInfo, which was called without template instantiation. In contrast, TClingClassInfo::TClingClassInfo(cling::Interpreter *interp, const char *name, bool instantiateTemplate /* = true */) is (almost always) called with instantiateTemplate set to true (unless "IsUnloading").

So maybe this issue is a won't fix as the calls are fundamentally different and not reusable?

@phsft-bot

This comment was marked as outdated.

Copy link

github-actions bot commented Feb 24, 2024

Test Results

     6 files      6 suites   2d 5h 28m 1s ⏱️
 2 730 tests   851 ✅   5 💤  1 874 ❌
15 624 runs  4 863 ✅ 372 💤 10 389 ❌

For more details on these failures, see this check.

Results for commit b56c8d3.

♻️ This comment has been updated with latest results.

@pcanal
Copy link
Member

pcanal commented Apr 5, 2024

trying to reset new CI

@pcanal pcanal closed this Apr 5, 2024
@pcanal pcanal reopened this Apr 5, 2024
@pcanal
Copy link
Member

pcanal commented Apr 5, 2024

@phsft-bot build

@ferdymercury
Copy link
Collaborator Author

trying to reset new CI

Maybe rebasing and force pushing does the trick?

@pcanal
Copy link
Member

pcanal commented Apr 5, 2024

Closing and reopening sorta worked: https://github.com/root-project/root/actions/runs/8574933039/job/23502827111?pr=14760 but the 'check' page is not seeing them. So far the failure are 'only' failed S3 connections.

@ferdymercury ferdymercury marked this pull request as draft April 8, 2024 08:27
@ferdymercury ferdymercury changed the title [cling] use one instead of two lookups within TClass::Init() [core] use one instead of two lookups within TClass::Init() Apr 15, 2024
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.

TClass::Init() does two lookups where one is enough
3 participants