Skip to content

Rename wasm-delegations[-fields].h to def files (NFC) #3941

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

Merged
merged 2 commits into from
Jun 18, 2021

Conversation

aheejin
Copy link
Member

@aheejin aheejin commented Jun 18, 2021

These files are special in that they use define symbols that are not
defined within those files or other files included in those files; they
are supposed to be defined in source files that include these headers.
This has caused clang-tidy to fail every time these files have changed
because they are not compilable per se.

This PR solves the problem by changing their extension to def, which
is also used in LLVM codebase. LLVM has dozens of files like this whose
extension is def, which makes these not checked by clang-tidy.

These files are special in that they use define symbols that are not
defined within those files or other files included in those files; they
are supposed to be defined in source files that include these headers.
This has caused clang-tidy to fail every time these files have changed
because they are not compilable per se.

This PR solves the problem by changing their extension to `def`, which
is also used in LLVM codebase. LLVM has dozens of files like this whose
extension is `def`, which makes this not checked by clang-tidy.
@aheejin aheejin requested review from kripken and tlively June 18, 2021 06:19
Copy link
Member

@tlively tlively left a comment

Choose a reason for hiding this comment

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

Good thinking! This is a much better solution than what I was thinking of :)

Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

Very nice!

@aheejin
Copy link
Member Author

aheejin commented Jun 18, 2021

@tlively Out of curiosity, what was the solution you were thinking of?

@aheejin aheejin merged commit 97e277c into WebAssembly:main Jun 18, 2021
@aheejin aheejin deleted the delegation_def branch June 18, 2021 20:49
@tlively
Copy link
Member

tlively commented Jun 18, 2021

The workarounds I mentioned in #3937 (comment). Downgrading the errors to warnings would have been an unfortunate loss of functionality and avoiding the errors in the special case where none of the macros are defined would have been unnecessarily complicated.

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