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

[UR] Check for unused parameters #861

Merged

Conversation

veselypeta
Copy link
Contributor

Since UR is imported as a library to different project, we should make sure we don't cause warnings with their compiler. A common warning is for unused parameters so we should ensure we don't raise this warning when compiling the code.

@veselypeta veselypeta force-pushed the petr/add-unused-parameter-checks branch from fc8e261 to e14e8bc Compare September 15, 2023 09:26
@@ -18,7 +18,7 @@
using namespace logger;

//////////////////////////////////////////////////////////////////////////
int main(int argc, char *argv[]) {
int main([[maybe_unused]] int argc, [[maybe_unused]] char *argv[]) {
Copy link
Contributor

Choose a reason for hiding this comment

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

I think in this case we should just remove these arguments instead.

Copy link
Contributor

@pbalcer pbalcer left a comment

Choose a reason for hiding this comment

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

There's no need to use [[maybe_unused]]. You can just omit the parameter name.

@veselypeta
Copy link
Contributor Author

There's no need to use [[maybe_unused]]. You can just omit the parameter name.

Do you mean in every occurrence or just the one Benie mentioned above? I thinking it ur_params.hpp it is necessary to use [[maybe_unused]] since the parameter only sometimes not used. I also think that keeping the parameter names keeps the context of what the function is doing, removing the names might make it more difficult later on to decipher what the parameter is used for?

@veselypeta veselypeta force-pushed the petr/add-unused-parameter-checks branch from e14e8bc to 747613b Compare September 18, 2023 15:03
@pbalcer
Copy link
Contributor

pbalcer commented Sep 18, 2023

There's no need to use [[maybe_unused]]. You can just omit the parameter name.

Do you mean in every occurrence or just the one Benie mentioned above? I thinking it ur_params.hpp it is necessary to use [[maybe_unused]] since the parameter only sometimes not used. I also think that keeping the parameter names keeps the context of what the function is doing, removing the names might make it more difficult later on to decipher what the parameter is used for?

My preference would be to get rid of it entirely except for the functions where the parameter is really "maybe unused" (like in ur_params.hpp). The most common scenario where a parameter is unused is when the function implements some sort of interface with a fixed set of parameters. In those cases, there's no reason to keep the parameter names because the interfaces are documented elsewhere. In other cases, we should consider whether a parameter should exist in the first place (e.g., like what Benie mentioned).

@veselypeta veselypeta merged commit e40a321 into oneapi-src:main Sep 19, 2023
26 checks passed
@veselypeta veselypeta deleted the petr/add-unused-parameter-checks branch October 26, 2023 10:04
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.

4 participants