[PATCH] D125402: [clang][diag] warn if function returns class type by-const-value

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D125402#3517865 <https://reviews.llvm.org/D125402#3517865>, @nlee wrote:

> 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:

I don't think that will be quite correct -- IIRC, that would still return true if the move constructor was deleted. `hasSimpleMoveConstructor()` and `hasSimpleMoveAssignment()` might be a better approach.

> @aaron.ballman Can you suggest some other projects to test? I'm thinking of these: protobuf, pytorch

Those are good; I'd also run it over LLVM itself. Boost would also be a good thing to test against.

> Thank you for your reviews, and sorry for the mess in my last comment. I'm new to clang/Phabricator and trying to get used to it :)

No problem, Phabricator takes some getting used to and the Clang code base is pretty large. :-)



================
Comment at: clang/lib/Sema/SemaType.cpp:5203
+          T.isConstQualified() &&
+          T->isStructureOrClassType()) {
+        const SourceLocation ConstLoc = D.getDeclSpec().getConstSpecLoc();
----------------
nlee wrote:
> nlee wrote:
> > 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.
> > 
> > 
> It seems it is not always possible to call `T->getAsCXXRecordDecl()->hasMoveConstructor()` here. `CXXRecordDecl::DefinitionData` is not populated if definition is not provided in the translation unit, such as in the following case:
> ```
> extern struct no_def s;
> const no_def f();
> ```
> `assert` fails with message: `"queried property of class with no definition"`
Yeah, we calculate move/copy constructor/assignment (triviality, etc) while building up the type for the class as we add declarations to it. So if we've not seen the definition yet, we won't be able to check that property.

That said, it might make sense to only trigger this diagnostic when *defining* the function because the parameter and return types must be complete by that point.


================
Comment at: clang/test/SemaCXX/warn-return-by-const-value.cpp:33
+}
\ No newline at end of file

----------------
You should add the newline to the end of the file.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D125402/new/

https://reviews.llvm.org/D125402