Skip to content

Allow ROOT_INCLUDE to specify standard library include paths#113

Closed
Y-- wants to merge 7 commits into
root-project:masterfrom
Y--:master
Closed

Allow ROOT_INCLUDE to specify standard library include paths#113
Y-- wants to merge 7 commits into
root-project:masterfrom
Y--:master

Conversation

@Y--
Copy link
Copy Markdown
Contributor

@Y-- Y-- commented Dec 4, 2015

This way we do not need g++ at runtime

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

We try to layer ROOT on top of cling, as cleanly as possible. Thus cling should use ROOT_INCLUDE. Can we maybe adjust the interpreter construction instead, passing -isystem and -nostdinc++? Then cling could skip the stdinc++ determination, reacting on -nostdinc++ being passed as an argument.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

That would be great and makes much more sense indeed.
Is it something that is already implemented or does it need to be?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Handling of -isystem should exist; skipping the gcc invocation if -nostdinc++ is passed or the ROOT part of the compiler includes do not exist.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

It works properly indeed.
Here is the new version : Y--@39736e1

@Axel-Naumann
Copy link
Copy Markdown
Member

Thank you for your PR! Nice ideas, let's get this in! Please let me know if one of my comments wasn't precise enough or misunderstood your patch.

@Y--
Copy link
Copy Markdown
Contributor Author

Y-- commented Dec 6, 2015

Thanks for your quick review!
I'll try to work on your suggestions asap so you can integrate them.
Also, do you want me to do the same PR on Cling project?

@Axel-Naumann
Copy link
Copy Markdown
Member

@Y-- - no need for an extra PR for cling! We will apply the changes from ROOT's cling to the cling repo.

@Axel-Naumann
Copy link
Copy Markdown
Member

@Y-- Indeed, last of -O3 -O0 wins.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Where's this needed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oups, I forgot to remove that. It's not used indeed.
Removed here : Y--@83e6fc7

@Axel-Naumann
Copy link
Copy Markdown
Member

Excellent! Merged as 10cf52d..73b3a1d into master. (I have squashed some of your commits.)

Thank you for your contributions!

@Y--
Copy link
Copy Markdown
Contributor Author

Y-- commented Dec 11, 2015

Awesome! Thank you so much :-)

hmalphettes added a commit to sutoiku/docker-debian-cling that referenced this pull request Dec 15, 2015
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