[PATCH] D41933: handle scoped_lockable objects being returned by value in C++17

[Aaron Ballman via Phabricator via cfe-commits]

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