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

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added inline comments.


================
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
----------------
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.


================
Comment at: lib/Analysis/ThreadSafety.cpp:2001
+  for (CXXConstructorDecl *Ctor : RD->ctors()) {
+    if (Ctor->isDeleted() || Ctor->getAccess() != AS_public)
+      continue;
----------------
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.


Repository:
  rC Clang

https://reviews.llvm.org/D41933