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

readability-redundant-string-cstr quick-fix causes invalid code #43965

Closed
firewave opened this issue Jan 22, 2020 · 8 comments
Closed

readability-redundant-string-cstr quick-fix causes invalid code #43965

firewave opened this issue Jan 22, 2020 · 8 comments
Labels
bugzilla Issues migrated from bugzilla clang-tidy

Comments

@firewave
Copy link

Bugzilla Link 44620
Resolution FIXED
Resolved on Aug 27, 2020 06:00
Version unspecified
OS Windows NT
Blocks #46070
CC @f00kat,@zmodem,@hokein
Fixed by commit(s) b99630e

Extended Description

#include

static void f2(std::string&&) {
}

static void f() {
std::string const s;
f2(s.c_str()); // readability-redundant-string-cstr warning
}

Applying the quick-fix to the code above causes it to fail as an rvalue is required.

@f00kat
Copy link
Contributor

f00kat commented Feb 4, 2020

Hi

Which option is correct?

  1. Check that "f2" argument declaration(std::string&&) isRValueReferenceType and do nothing. We will use the copy of c_str() in this case.
  2. Fix with "f2(std::move(s))"

@llvmbot
Copy link
Collaborator

llvmbot commented Jul 29, 2020

Drive by note, maybe this should explicitly create a copy of the string
f2(std::string(s))

@llvmbot
Copy link
Collaborator

llvmbot commented Jul 29, 2020

Reopening this as a regression was introduced when fixing another bug, There is a patch in place to fix that regression https://reviews.llvm.org/D84831

@llvmbot
Copy link
Collaborator

llvmbot commented Jul 29, 2020

Fixed again in b99630e, Will leave open for 11 branch merging as this regression was introduced on the 11 branch.

@zmodem
Copy link
Collaborator

zmodem commented Jul 29, 2020

Fixed again in
https://github.com/llvm/llvm-project/commit/
b99630e, Will leave open for 11 branch
merging as this regression was introduced on the 11 branch.

Pushed as b99630e

Please let me know if there are any further issues.

@hokein
Copy link
Collaborator

hokein commented Aug 27, 2020

reopen this for cherry-pick eed0af6

@hokein
Copy link
Collaborator

hokein commented Aug 27, 2020

oops, sorry, I changed a wrong bug.

@zmodem
Copy link
Collaborator

zmodem commented Nov 27, 2021

mentioned in issue #46070

@llvmbot llvmbot transferred this issue from llvm/llvm-bugzilla-archive Dec 10, 2021
This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugzilla Issues migrated from bugzilla clang-tidy
Projects
None yet
Development

No branches or pull requests

5 participants