[PATCH] D92733: Fix PR25627 - false positive diagnostics involving implicit-captures in dependent lambda expressions.

[Wyatt Childers via Phabricator via cfe-commits]

wchilders requested changes to this revision.
wchilders added a comment.
This revision now requires changes to proceed.

I'm still "chewing on this", I'm not sure I fully understand the problem well enough to give great "big picture" feedback. In any case, I've left a few comments where I had a potentially helpful thought :)



================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:7743
+      const VarDecl *StaticDataMember = dyn_cast<VarDecl>(E->getMemberDecl());
+      if (StaticDataMember) {
+        Results.push_back(E);
----------------
This might be better restated as:

```
if (isa<VarDecl>(E->getMemberDecl())) {
```


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:7754
+      Visit(E->getBase());
+      VisitingArraySubscriptBaseExpr = false;
+    }
----------------
This likely should restore the previous state to prevent preemptive clearing of the flag, i.e.
```
bool WasVisitingArraySubscriptBase = VisitingArraySubscriptBaseExpr;
VisitingArraySubscriptBaseExpr = true;
Visit(E->getBase());
VisitingArraySubscriptBaseExpr = WasVisitingArraySubscriptBase;
```

I haven't given sufficient thought as to whether that would be problematic in this case; but it seems worth bringing up.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:7813
+          CheckLValueToRValueConversionOperand(E, /*DiscardedValue=*/true);
+      E = ER.get();
     }
----------------
This should probably handle the error case, no?


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:7881
+  if (Var->getType()->isDependentType())
+    return false;
 
----------------
Could you elaborate on the changes made to this function -- if for no other reason than my curiosity :)? This seems to be largely the same checks, in a different order and some control flow paths that used to return false now potentially return true and vice versa.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:8016
+        IsNonCaptureableLambda && IsInitializingAnEntityWithDependentType &&
+        !IsReferenceRelatedAutoTypeDeduction) {
+
----------------
Is there a succinct way to rewrite this that might improve readability? i.e. 

```
bool SuccinctName = !IsFullExprInstantiationDependent && !IsVarNeverAConstantExpression &&
        IsNonCaptureableLambda && IsInitializingAnEntityWithDependentType &&
        !IsReferenceRelatedAutoTypeDeduction;
if (SuccinctName) {
```

Alternatively a short comment; It's pretty hard (at least for me) to tell at a glance what this branch is for.


================
Comment at: clang/lib/Sema/SemaExprCXX.cpp:8037
+    //   2) the VarDecl can never be a constant expression. 
+    //   3) we  are initializing to a reference-related type (via auto)
+    //   4) we are  initializing to the declaration type of the VarDecl, and
----------------
Minor, but there's an extra space here the pre-merge didn't catch


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92733