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

avoid multiple redefinition #1817

Merged
merged 13 commits into from
Dec 14, 2023
Merged

avoid multiple redefinition #1817

merged 13 commits into from
Dec 14, 2023

Conversation

MitchellX
Copy link
Contributor

By including these guards, you ensure that if ops.h is included multiple times in different files, its contents will only be processed once by the compiler, avoiding redefinition errors and saving time

@simon-mo
Copy link
Collaborator

thank you for your contribution! can you check is this the case for all other header files

@WoosukKwon
Copy link
Collaborator

@MitchellX Thanks for the PR! As Simon mentioned, it seems there are several header files that do not have the guards. Could you please add the guards to all theses header files?

@MitchellX
Copy link
Contributor Author

@simon-mo @WoosukKwon Sure, I'll do it later.

@MitchellX MitchellX closed this Nov 29, 2023
@MitchellX MitchellX reopened this Nov 29, 2023
@WoosukKwon
Copy link
Collaborator

@MitchellX Are you still working on this PR?

@MitchellX
Copy link
Contributor Author

@WoosukKwon My apologies for the oversight. I was so overwhelmed the last few weeks, forgetting to modify the head files. Yes, I'm still working on a vllm related project, and I can help address this issue on Saturday.

@WoosukKwon
Copy link
Collaborator

@MitchellX I see. Thanks for letting us know. In this case, can I directly modify this PR to accelerate the process? I think we got your point and just needed to add #pragma once to other header files. I can take this if you are ok with it.

@MitchellX
Copy link
Contributor Author

@WoosukKwon Sure, do as you wish.

Copy link
Collaborator

@WoosukKwon WoosukKwon left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for pointing this out!

@WoosukKwon WoosukKwon merged commit 614856d into vllm-project:main Dec 14, 2023
2 checks passed
xjpang pushed a commit to xjpang/vllm that referenced this pull request Dec 18, 2023
hongxiayang pushed a commit to hongxiayang/vllm that referenced this pull request Feb 13, 2024
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.

None yet

3 participants