Skip to content

Don't use std::ignore for unused parameters #17219

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
Apr 15, 2025

Conversation

ldrumm
Copy link
Contributor

@ldrumm ldrumm commented Feb 27, 2025

std::ignore seems to have been adopted as some sort of ersatz (void) cast or [[maybe_unused]] annotation on unused parameters. Since the following are true:

  1. The behavior of std::ignore outside of std::tie is not formally specified;
  2. C++ already has builtin ways to express ignorance of a value;
  3. Compiling std::tie is much more expensive than using the builtin alternatives [1];
  4. It's confusing to read;
  5. It requires an extra header inclusion: I decided to simply omit the argument name in all cases where this is possible.

This is a mostly completely mechanical transformation, using the following script:

find unified-runtime -name "*.[ch]pp" \
  | xargs sed -i '/^\s*std::ignore\s*=\s*[a-zA-Z_][a-zA-Z0-9_]*;/d' \
  &&  git ls-files -m | parallel clang-tidy \
    '--checks="-*,misc-unused-parameters"' --fix --fix-errors -p build

Followed by manually introducting [[maybe_unused]] in NDEBUG blocks in a few places.

[1]: Both clang and gcc with libstdc++ and libc++ show similar slowdowns compiling the std::ignore vs void.
I didn't try with MSVC but I see no reason it wouldn't be similar

$ ipython3
In [1]: def ignore(n):
   ...:     ignores = 'std::ignore = x;\n' * n
   ...:     return f'''#include <tuple>\nint foo(int x) {{ {ignores} \n return x;}}'''
   ...:

In [2]: def void(n):
   ...:     voids = '(void)x;\n' * n
   ...:     return f'''int foo(int x) {{ {voids} \n return x;\n}}'''
   ...:

In [3]: with open("ignore.cpp", "w") as f:
   ...:     f.write(ignore(100000))
   ...:

In [4]: with open("voids.cpp", "w") as f:
   ...:     f.write(void(100000))
   ...:
$ time c++ -S voids.cpp
c++ -S voids.cpp  0.18s user 0.03s system 99% cpu 0.211 total
$ time c++ -S ignore.cpp
c++ -S ignore.cpp  4.50s user 0.34s system 99% cpu 4.836 total

@ldrumm ldrumm requested review from a team as code owners February 27, 2025 14:00
@ldrumm ldrumm force-pushed the down-with-this-sort-of-thing branch from 82a7af9 to abfd946 Compare February 27, 2025 14:07
@ldrumm ldrumm force-pushed the down-with-this-sort-of-thing branch from abfd946 to c38dde1 Compare February 27, 2025 14:11
@ldrumm ldrumm force-pushed the down-with-this-sort-of-thing branch 2 times, most recently from 58b63a7 to 7ca2018 Compare February 27, 2025 17:08
@ldrumm
Copy link
Contributor Author

ldrumm commented Apr 15, 2025

ping @intel/dpcpp-nativecpu-reviewers, @intel/sycl-graphs-reviewers, @intel/unified-runtime-reviewers, and/or @intel/unified-runtime-reviewers-level-zero

ldrumm added 2 commits April 15, 2025 11:23
`std::ignore` seems to have been adopted as some sort of ersatz `(void)`
cast or `[[maybe_unused]]` annotation on unused parameters.
Since the following are true:
  1. The behavior of `std::ignore` outside of `std::tie` is not formally
     specified;
  2. C++ already has builtin ways to express ignorance of a value;
  3. Compiling `std::tie` is much more expensive than using the builtin
     alternatives [1];
  4. It's confusing to read;
  5. It requires an extra header inclusion:
I decided to simply omit the argument name in all cases where this is
possible.

This is a mostly mechanical transformation, using the following
script:
```
find unified-runtime -name "*.[ch]pp" \
  | xargs sed -i '/^\s*std::ignore\s*=\s*[a-zA-Z_][a-zA-Z0-9_]*;/d' \
  &&  git ls-files -m | parallel clang-tidy \
    '--checks="-*,misc-unused-parameters"' --fix --fix-errors -p build
```
followed by manual fixup around conditional compilation blocks.

[1]: Both clang and gcc with libstdc++ and libc++ show similar slowdowns
compiling the std::ignore vs void.
I didn't try with MSVC but I see no reason it wouldn't be similar
```
$ ipython3
In [1]: def ignore(n):
   ...:     ignores = 'std::ignore = x;\n' * n
   ...:     return f'''#include <tuple>\nint foo(int x) {{ {ignores} \n return x;}}'''
   ...:

In [2]: def void(n):
   ...:     voids = '(void)x;\n' * n
   ...:     return f'''int foo(int x) {{ {voids} \n return x;\n}}'''
   ...:

In [3]: with open("ignore.cpp", "w") as f:
   ...:     f.write(ignore(100000))
   ...:

In [4]: with open("voids.cpp", "w") as f:
   ...:     f.write(void(100000))
   ...:
$ time c++ -S voids.cpp
c++ -S voids.cpp  0.18s user 0.03s system 99% cpu 0.211 total
$ time c++ -S ignore.cpp
c++ -S ignore.cpp  4.50s user 0.34s system 99% cpu 4.836 total
```
@ldrumm ldrumm force-pushed the down-with-this-sort-of-thing branch from c39bff0 to 18b01d9 Compare April 15, 2025 10:25
@ldrumm ldrumm temporarily deployed to WindowsCILock April 15, 2025 10:25 — with GitHub Actions Inactive
@ldrumm ldrumm temporarily deployed to WindowsCILock April 15, 2025 13:39 — with GitHub Actions Inactive
Copy link
Contributor

@Bensuo Bensuo left a comment

Choose a reason for hiding this comment

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

Graphs/command-buffer changes LGTM

@ldrumm ldrumm requested a review from hvdijk April 15, 2025 17:16
Copy link
Contributor

@hvdijk hvdijk left a comment

Choose a reason for hiding this comment

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

NativeCPU looks good, thanks.

@ldrumm ldrumm merged commit 82d8ed7 into intel:sycl Apr 15, 2025
38 of 39 checks passed
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.

7 participants