[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

cor3ntin added a comment. > The issue is that for its own tests, libc++ headers are not considered system > headers. we should fix libc++. I can do that later today. I though > @Quuxplusone did. Patch here https://reviews.llvm.org/D120509 - I'm still running the test locally to double check

Re: [PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

I thought I caught all of them, too! I encourage someone with commit rights to land the missing five characters "std::" right now, and/or I'll be able to take my own look in about 6 hours from now. Arthur On Thu, Feb 24, 2022, 2:06 PM Corentin Jabot via Phabricator < revi...@reviews.llvm.org>

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

cor3ntin added a comment. In D119670#3343726 , @erichkeane wrote: > In D119670#3343718 , @rnk wrote: > >> This broke our libc++ -Werror build: >> >>

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

erichkeane added a comment. In D119670#3343718 , @rnk wrote: > This broke our libc++ -Werror build: > > llvm-project/libcxx/src/filesystem/directory_iterator.cpp:107:57: error: > unqualified call to std::move [-Werror,-Wunqualified-std-cast-call] >

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

rnk added a comment. This broke our libc++ -Werror build: llvm-project/libcxx/src/filesystem/directory_iterator.cpp:107:57: error: unqualified call to std::move [-Werror,-Wunqualified-std-cast-call] __root_(move(other.__root_)),

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG70b1f6de5398: [clang] Warn on unqualified calls to std::move and std::forward (authored by cor3ntin, committed by erichkeane). Repository: rG

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

cor3ntin added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:6441 + // Only warn for some functions deemed more frequent or problematic + static constexpr llvm::StringRef SpecialFunctions[] = {"move", "forward"}; + auto it = llvm::find(SpecialFunctions,

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

cor3ntin added a comment. In D119670#3343076 , @erichkeane wrote: > I understand you still don't have commit-rights. If you'd like, I can push > this in a little bit. Just let me know the name/email you'd like me to use. I guess i should ask for

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

erichkeane added a comment. I understand you still don't have commit-rights. If you'd like, I can push this in a little bit. Just let me know the name/email you'd like me to use. Comment at: clang/lib/Sema/SemaExpr.cpp:6441 + // Only warn for some functions deemed more

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

cor3ntin added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:6441 + // Only warn for some functions deemed more frequent or problematic + static constexpr llvm::StringRef SpecialFunctions[] = {"move", "forward"}; + auto it = llvm::find(SpecialFunctions,

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

cor3ntin updated this revision to Diff 41. cor3ntin added a comment. Fix comments punctuation. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119670/new/ https://reviews.llvm.org/D119670 Files:

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

erichkeane accepted this revision. erichkeane added a comment. This revision is now accepted and ready to land. See the suggestions, our comments are supposed to be full sentences terminated with a full-stop. Otherwise nothing actionable. Comment at:

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

cor3ntin added inline comments. Comment at: clang/test/SemaCXX/warn-self-move.cpp:20-21 using std::move; - x = move(x); // expected-warning{{explicitly moving}} + x = move(x); // expected-warning{{explicitly moving}} \ + expected-warning {{unqualified

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

cor3ntin updated this revision to Diff 411101. cor3ntin added a comment. Fix comment to address Christopher's feedback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D119670/new/ https://reviews.llvm.org/D119670 Files:

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

cor3ntin updated this revision to Diff 411100. cor3ntin marked an inline comment as done. cor3ntin added a comment. - Adds more tests for calls inside std, inline namespaces and adl lookup - use llvm::find - rename the warning flag to unqualified-std-cast-call to leave room for future extensions

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

cor3ntin added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:6437 + NamedDecl *D = dyn_cast_or_null(Call->getCalleeDecl()); + if (!D || !D->isInStdNamespace()) +return; Quuxplusone wrote: > cor3ntin wrote: > > erichkeane wrote: > > > Quuxplusone

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

Quuxplusone added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:6437 + NamedDecl *D = dyn_cast_or_null(Call->getCalleeDecl()); + if (!D || !D->isInStdNamespace()) +return; cor3ntin wrote: > erichkeane wrote: > > Quuxplusone wrote: > > >

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

cor3ntin added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:6437 + NamedDecl *D = dyn_cast_or_null(Call->getCalleeDecl()); + if (!D || !D->isInStdNamespace()) +return; erichkeane wrote: > Quuxplusone wrote: > > erichkeane wrote: > > > Do we

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

erichkeane added inline comments. Comment at: clang/lib/Sema/SemaExpr.cpp:6437 + NamedDecl *D = dyn_cast_or_null(Call->getCalleeDecl()); + if (!D || !D->isInStdNamespace()) +return; Quuxplusone wrote: > erichkeane wrote: > > Do we really want this? I

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

Quuxplusone added a comment. Tangential data point: I hacked up this check to find //all// unqualified `std` functions and ran it on libc++'s tests and also on some of my own codebase (including re2, asio, protobuf). I'm fixing the issues it turned up in libc++'s tests. In the other codebases,

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

erichkeane added a comment. I'm not sure this is a 'good' enough warning (by design?) to allow in -Wall either, Wall seems to be only "this is definitely a mistake" type issues, and I don't think this issue rises to that requirement (though others are likely more qualified than I to judge).

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

cjdb added a comment. Thanks for working on this! I'm overall very happy, but I don't see a test case for a unary `move` originating from another namespace. Comment at: clang/lib/Sema/SemaExpr.cpp:6422 +static void DiagnosedUnqualifiedCallsToStdFunctions(Sema , CallExpr

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

Quuxplusone added inline comments. Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4812 +def warn_unqualified_call_to_std_function : Warning< + "unqualified call to %0">, InGroup>; + This should probably be named `-Wunqualified-std-move` resp.

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

cor3ntin added a comment. There may be some things worth considering here: - libc++ own tests and build is affected, I have not addressed these yet. - other standard libs are affected but because they are considered system headers, no warning is emitted I think this is a worth while first

[PATCH] D119670: [clang] Warn on unqualified calls to std::move and std::forward

cor3ntin created this revision. cor3ntin requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This adds a diagnostic when an unqualified call is resolved to std::move or std::forward. This follows some C++ committee discussions where some