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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
