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