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
[sycl-post-link][NFC] Address clang-tidy concerns in the sycl-post-link #5552
Conversation
The patch fixes some issues from clang-tidy related to the code-style and missed auto-stars (auto*) when a local variable is a pointer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Always nice with a bit of early spring cleaning. LGTM! 😄
|
||
if (TraverseResult.hasValue()) | ||
return std::move(*TraverseResult); | ||
return std::move(*TraverseResult); // TODO remove std::move after C++17 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why after C++17? Isn't it work since C++11?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I should remove this comment at all. My intent was the following, in C++17 copy elision is guaranteed by the Standard and there is no need in std::move
here (before C++17 compilers do such optimization too but it was not guaranteed). Usually compilers warn about using std::move
in return statements since it prevents RVO/NRVO optimization but not in this case, there is no warning during sycl-post-link
compilation at least on MSVC. I've tried a small example, a function that returns a vector of verbose copyable objects, but I see no copies regardless of std::move
using. @maksimsab Could you comment the using of std::move
here? I understand why std::move
is used in the last return statement there https://github.com/intel/llvm/pull/5552/files#diff-7d1ae2c6e3e847852b55edea2a54fc5309328f32e56800a636a4e3d9c906948bR414 (we just fill the instance of llvm::Optional
) but not here.
…/llvm into refactor_existing_workflows * 'refactor_existing_workflows' of github.com:alexbatashev/llvm: (2051 commits) [SYCL][L0] Honor property::queue::enable_profiling (intel#5543) [SYCL][CI] Enable sccache on Windows (intel#5589) [SYCL][Doc] Move internal design docs (intel#5556) [sycl-post-link] Initialize the integer Value variable (intel#5585) [CI] Fix nightly builds (intel#5584) [SYCL][L0] Fix use of copy-engines in L0 interop queue (intel#5579) Update OpenCL headers tag to dcd5bed (intel#5575) [SYCL] Fix warning for InOrderQueueSyncCheck unit test build (intel#5577) [SYCL][HIP] Remove arch requirement for running lit tests (intel#5253) [SYCL][L0] Fix timestamp calculation (in ns) (intel#5555) [SYCL] Fix sync of host task vs kernel for in-order queue (intel#5551) [sycl-post-link] Add a check for device globals with device_image_scope (intel#5517) [SYCL] Fix SYCL Kernel Body Check (intel#5546) [SYCL] Add support for SYCL 2020 in class group (intel#5447) Fix tests after 1c729d7 Use align attribute for kernel pointer arg alignment Fix tests after 18834dc Mark pointer-typed kernel arguments as ABI aligned [CI] Add experimental Windows build to GitHub Actions nightly (intel#5560) [sycl-post-link][NFC] Address clang-tidy concerns in the sycl-post-link (intel#5552) Fix lit test after changes in Clang Improve backward compatibility for DISubRange ...
The patch fixes some issues from clang-tidy related to the code-style
and missed auto-stars (auto*) when a local variable is a pointer.