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

Restoring old config file #150

Merged
merged 3 commits into from
Jun 18, 2024
Merged

Conversation

anutosh491
Copy link
Collaborator

Description

Addressing this comment raised by Johan

Type of change

Please tick all options which are relevant.

  • Bug fix
  • New feature
  • Added/removed dependencies
  • Required documentation updates

@anutosh491 anutosh491 changed the title Config Restoring old config file Jun 18, 2024
Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@anutosh491 anutosh491 requested a review from JohanMabille June 18, 2024 11:51
Copy link
Collaborator

@JohanMabille JohanMabille left a comment

Choose a reason for hiding this comment

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

I think we want to keep the XEUS_CPP_VERSION_LABEL token, and read it from the config file like the other ones.

Copy link
Contributor

clang-tidy review says "All clean, LGTM! 👍"

@vgvassilev
Copy link
Contributor

I am lost. Why do we undo the work being done in this area?

@JohanMabille
Copy link
Collaborator

As said in the original PR, the building tool should not inject anything in the source code, this makes the code depending on the building tool. The original PR was merged to get the inspect feature work until we find a better solution. Now that we have one, we don't need the build tool to inject anything in the source code anymore and we can revert to the original behavior.

@vgvassilev
Copy link
Contributor

As said in the original PR, the building tool should not inject anything in the source code, this makes the code depending on the building tool. The original PR was merged to get the inspect feature work until we find a better solution. Now that we have one, we don't need the build tool to inject anything in the source code anymore and we can revert to the original behavior.

Ok, good. I misread the pull request on my phone. Apologies. LGTM!

@JohanMabille JohanMabille merged commit 4745f72 into compiler-research:main Jun 18, 2024
9 checks passed
@anutosh491 anutosh491 deleted the config branch June 19, 2024 05:02
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.

3 participants