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

clang-tidy sends false positive warnings #18617

Open
tchaikov opened this issue May 10, 2024 · 6 comments · May be fixed by #20970
Open

clang-tidy sends false positive warnings #18617

tchaikov opened this issue May 10, 2024 · 6 comments · May be fixed by #20970
Assignees
Labels
area/build backport/none Backport is not required

Comments

@tchaikov
Copy link
Contributor

tchaikov commented May 10, 2024

for instance:

return open_file_dma(path.native(), oflags, std::move(options)).then([this, path = std::move(path)] (file f) mutable {
   99 |     return open_file_dma(path.native(), oflags, std::move(options)).then([this, path = std::move(path)] (file f) mutable {
      |                          ^
/home/runner/work/scylladb/scylladb/seastar/src/util/tmp_file.cc:99:88: note: move occurred here
   99 |     return open_file_dma(path.native(), oflags, std::move(options)).then([this, path = std::move(path)] (file f) mutable {
      |                                                                                        ^
/home/runner/work/scylladb/scylladb/seastar/src/util/tmp_file.cc:99:26: note: the use and move are unsequenced, i.e. there is no guarantee about the order in which they are evaluated
   99 |     return open_file_dma(path.native(), oflags, std::move(options)).then([this, path = std::move(path)] (file f) mutable {
      |                          ^
Warning: /home/runner/work/scylladb/scylladb/seastar/src/util/tmp_file.cc:146:28: warning: 'path' used after it was moved [bugprone-use-after-move]

this is a known issue of clang-tidy.

see llvm/llvm-project#57758 and llvm/llvm-project#59612. and there is a fix for it, see https://reviews.llvm.org/D145581 . but this patch is now stalled, and has not landed on llvm yet.

@tchaikov
Copy link
Contributor Author

i am creating this issue just for those who are interested in the false alarms.

@tchaikov tchaikov closed this as not planned Won't fix, can't repro, duplicate, stale May 10, 2024
@tchaikov
Copy link
Contributor Author

/cc @raphaelsc

@tchaikov
Copy link
Contributor Author

tchaikov commented May 28, 2024

created llvm/llvm-project#93623 based on https://reviews.llvm.org/D145581 to address the false alarms.

@raphaelsc
Copy link
Member

created llvm/llvm-project#93623 based on https://reviews.llvm.org/D145581 to address the false alarm

great initiative @tchaikov. thanks for submitting it.

@tchaikov
Copy link
Contributor Author

tchaikov commented Jul 8, 2024

merged. lemme see if it is allowed to be backported after it passes the tests

@tchaikov tchaikov reopened this Sep 30, 2024
@tchaikov
Copy link
Contributor Author

tchaikov commented Sep 30, 2024

reopening this issue. because we should be able to use clang-19 which includes the fix . but better off using a distro which has clang-19 prepackaged.

@tchaikov tchaikov self-assigned this Sep 30, 2024
tchaikov added a commit to tchaikov/scylladb that referenced this issue Oct 5, 2024
since fedora 41 has received the llvm 41 packages. let's use
it.

Fixes scylladb#18617
Signed-off-by: Kefu Chai <[email protected]>
tchaikov added a commit to tchaikov/scylladb that referenced this issue Oct 5, 2024
since fedora 41 has received the llvm 41 packages. let's use
it.

Fixes scylladb#18617
Signed-off-by: Kefu Chai <[email protected]>
@tchaikov tchaikov linked a pull request Oct 5, 2024 that will close this issue
@github-actions github-actions bot added the backport/none Backport is not required label Oct 5, 2024
tchaikov added a commit to tchaikov/scylladb that referenced this issue Oct 7, 2024
since fedora 41 has received the llvm 41 packages. let's use
it.

Fixes scylladb#18617
Signed-off-by: Kefu Chai <[email protected]>
tchaikov added a commit to tchaikov/scylladb that referenced this issue Oct 7, 2024
since fedora 41 has received the llvm 41 packages. let's use
it.

Fixes scylladb#18617
Signed-off-by: Kefu Chai <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/build backport/none Backport is not required
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants