aaron.ballman accepted this revision.
aaron.ballman added a comment.
This revision is now accepted and ready to land.
LGTM, but you should wait for a bit to see if DeLesley has concerns.
================
Comment at: lib/Analysis/ThreadSafety.cpp:1994
+static CXXConstructorDecl *findConstructorForByValueReturn(CXXRecordDecl *RD) {
+ // Prefer a move constructor over a copy constructor. If there's more than
----------------
rsmith wrote:
> aaron.ballman wrote:
> > Can you sprinkle some const correctness around this declaration?
> I can't make the `CXXConstructorDecl` const without adding a `const_cast`
> somewhere or making very large-scale changes to Clang :) The
> `CXXConstructExpr` constructor needs a pointer to a non-const
> `CXXConstructorDecl`.
>
> I can make the `CXXRecordDecl` const. I think that's actually a
> const-correctness bug -- if `const` on an AST node means anything, it should
> mean that the AST is potentially immutable, and you shouldn't be able to get
> from a const AST node to a non-const one -- but it's probably closer to the
> ideal than no const, so I'll add a const there.
Thanks! I agree with you that `const` on an AST node should mean that the AST
is immutable, but I think getting to that point would be a pretty large change
to Clang in general, unfortunately.
================
Comment at: lib/Analysis/ThreadSafety.cpp:2001
+ for (CXXConstructorDecl *Ctor : RD->ctors()) {
+ if (Ctor->isDeleted() || Ctor->getAccess() != AS_public)
+ continue;
----------------
rsmith wrote:
> aaron.ballman wrote:
> > Should we care about access checking here (protected ctor within a
> > reasonable context, or friendship)?
> I'm not completely sure. My thinking was that if someone uses the "private
> copy constructor with no definition" approach rather than the "deleted copy
> constructor" approach, we should ignore that copy constructor. But this would
> only really matter if they do that to their *move* constructor and they have
> an accessible copy constructor, which seems like a sufficiently bizarre
> situation to not have a special rule for it. (Well, it'd also matter if the
> private copy ctor with no definition had thread safety attributes, which also
> seems like a very strange case.)
>
> Happy to go either way here. On balance I think I agree that it's more
> consistent with the general language rules to ignore access here.
I think ignoring access is a good first approximation. We can refine later if
we find the analysis requires it.
Repository:
rC Clang
https://reviews.llvm.org/D41933
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
