nlee added a comment.
In D125402#3508845 <https://reviews.llvm.org/D125402#3508845>, @aaron.ballman
wrote:
> Thank you for working on this new diagnostic! We don't typically add new
> diagnostics that are off by default unless they're for pedantic diagnostics
> or are reasonably expected to be enabled by default in the future. This is
> because we've had evidence that suggests such diagnostics aren't enabled
> often enough to warrant the maintenance cost for them.
>
> I'm not convinced this diagnostic can be enabled by default. Yes, it prevents
> move semantics, but that's not typically an issue with the correctness of the
> code. IIRC, we decided to put this diagnostic into clang-tidy because we
> thought it would be too chatty in practice, especially on older code bases.
>
> Perhaps there are ways we can improve the diagnostic behavior to allow it to
> be enabled by default though. One possibility would be to look at the return
> type to see if there's a user-provided move constructor (either `= default`
> or explicitly written) and only diagnose if one is found. But I think we'd
> want some evidence that this actually reduces the false positive rate in
> practice, which means trying to compile some large C++ projects (of various
> ages/code quality) to see. Would you be willing to run your patch against
> some large C++ projects to see what kind of true and false positives it
> generates? From there, we can decide whether we need additional heuristics or
> not.
How about adding CXXRecordDecl::hasMoveConstructor() to ensure the type is
movable? I ran a test with OpenCV(c++11), and it returned some useful warnings:
In file included from
/Users/nlee/Documents/opencv/modules/core/include/opencv2/core/base.hpp:662:
/Users/nlee/Documents/opencv/modules/core/include/opencv2/core/check.hpp:16:12:
warning: returning by const value prevents move semantics
[-Wreturn-by-const-value]
CV_EXPORTS const String typeToString(int type);
^~~~~~
/Users/nlee/Documents/opencv/modules/core/include/opencv2/core/check.hpp:26:12:
warning: returning by const value prevents move semantics
[-Wreturn-by-const-value]
CV_EXPORTS const cv::String typeToString_(int type);
^~~~~~
@aaron.ballman Can you suggest some other projects to test? I'm thinking of
these: protobuf, pytorch
> I wonder if this is same concern applies to Unions? Should this just be
> `T->isRecordType()`? Should we do more analysis to ensure that this is a
> movable type? I THINK `CXXRecordDecl::defaultedMoveConstructorIsDeleted()`
> should be enough to test for t his.
@erichkeane Does adding `CXXRecordDecl::hasMoveConstructor()` suffices to
ensure the type is movable? I left out unions because they may alert false
positives. An example of such a case is:
union U { int i; std::string s; };
const U f() { return U{239}; }
================
Comment at: clang/lib/Sema/SemaType.cpp:5203
+ T.isConstQualified() &&
+ T->isStructureOrClassType()) {
+ const SourceLocation ConstLoc = D.getDeclSpec().getConstSpecLoc();
----------------
erichkeane wrote:
> I wonder if this is same concern applies to Unions? Should this just be
> `T->isRecordType()`? Should we do more analysis to ensure that this is a
> movable type? I THINK `CXXRecordDecl::defaultedMoveConstructorIsDeleted()`
> should be enough to test for t his.
How about adding `CXXRecordDecl::hasMoveConstructor()` to ensure the type is
movable? I left out unions because they may alert false positives. An example
of such a case is:
```
union U { int i; std::string s; };
const U f() { return U{239}; }
```
================
Comment at: clang/lib/Sema/SemaType.cpp:5203
+ T.isConstQualified() &&
+ T->isStructureOrClassType()) {
+ const SourceLocation ConstLoc = D.getDeclSpec().getConstSpecLoc();
----------------
nlee wrote:
> erichkeane wrote:
> > I wonder if this is same concern applies to Unions? Should this just be
> > `T->isRecordType()`? Should we do more analysis to ensure that this is a
> > movable type? I THINK `CXXRecordDecl::defaultedMoveConstructorIsDeleted()`
> > should be enough to test for t his.
> How about adding `CXXRecordDecl::hasMoveConstructor()` to ensure the type is
> movable? I left out unions because they may alert false positives. An example
> of such a case is:
> ```
> union U { int i; std::string s; };
> const U f() { return U{239}; }
> ```
> I wonder if this is same concern applies to Unions? Should this just be
> `T->isRecordType()`? Should we do more analysis to ensure that this is a
> movable type? I THINK `CXXRecordDecl::defaultedMoveConstructorIsDeleted()`
> should be enough to test for t his.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125402/new/
https://reviews.llvm.org/D125402
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
