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

[Namgoo Lee via Phabricator via cfe-commits]

nlee added a comment.

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



================
Comment at: clang/lib/Sema/SemaType.cpp:5203
+          T.isConstQualified() &&
+          T->isStructureOrClassType()) {
+        const SourceLocation ConstLoc = D.getDeclSpec().getConstSpecLoc();
----------------
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"`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D125402