Skip to content
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

[interpreter] new function TInterpreter::SetIncludePath #17412

Closed
wants to merge 4 commits into from

Conversation

ferdymercury
Copy link
Contributor

@ferdymercury ferdymercury commented Jan 10, 2025

This Pull request:

Changes or fixes:

Fixes https://its.cern.ch/jira/browse/ROOT-5149

This needs a patch on top of LLVM, so maybe it's better to just close this issue as "won't fix" ?

Checklist:

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

This PR fixes #

Copy link

github-actions bot commented Jan 10, 2025

Test Results

0 tests   0 ✅  0s ⏱️
0 suites  0 💤
0 files    0 ❌

Results for commit 3431038.

♻️ This comment has been updated with latest results.

@dpiparo
Copy link
Member

dpiparo commented Jan 12, 2025

Thanks a lot for this code, impressive. I would like to ask the expert advice of @devajithvs about the need of a clang patch. We are making a lot of effort to remove patches, and adding a patch for this feature might seem perhaps an overkill (the feature would be nice, but it's somewhat far from being a blocker for anybody, I think)

@ferdymercury
Copy link
Contributor Author

Thanks a lot for this code, impressive. I would like to ask the expert advice of @devajithvs about the need of a clang patch. We are making a lot of effort to remove patches, and adding a patch for this feature might seem perhaps an overkill (the feature would be nice, but it's somewhat far from being a blocker for anybody, I think)

Yep, I completely agree. My vote is for closing the JIRA issue as 'wont-fix' (as well as this PR :) ).

@dpiparo
Copy link
Member

dpiparo commented Jan 13, 2025

After discussing the matter with llvm experts, team members and the author of this PR, we converged on the decision of closing this PR. The changes are quite well done, however in disagreement with the general direction of the project, which is to reduce the number of patches, ideally removing all of them. Once again, thanks for this excellent proposal for code changes.

@dpiparo dpiparo closed this Jan 13, 2025
@ferdymercury ferdymercury deleted the setincludepath branch January 13, 2025 16:30
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.

2 participants