To group the code in one place, simplify it and make it easier to add
the containsErrors bit and find existing bugs.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
@rsmith this does not pass through testing, I've messed up somewhere while moving the code.
I'll find what went wrong and fix it tomorrow. Please tell if the approach itself LG.
Unit tests: unknown.
clang-tidy: unknown.
clang-format: unknown.
Build artifacts: diff.json, console-log.txt
Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.
Unit tests: unknown.
clang-tidy: unknown.
clang-format: unknown.
Build artifacts: diff.json, console-log.txt
Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.
This patch contains too many changes, most of them are just NFC, it likely takes a long time to do a full review.
I actually did an review for the original patch. I have highlighted places (see me comments) that I'm uncertain and need another look, other places are NFC.
clang/include/clang/AST/Expr.h | ||
---|---|---|
1539 | this is not needed indeed as the default value is 0, but I'd keep it here to make it more explicit. | |
4092 | ! behavior change change, now the ConvertVectorExpr::TypeDependent = DstType->isDependentType() | SrcExpr->isDependentType(). | |
4150 | ! this needs careful review, the ChooseExpr bits are from different sources:
| |
4261 | ! the implementation in the original patch doesn't seem to correct to me, I rewrote it, need another review.. | |
5042 | ! behavior change here, now ArrayInitLoopExpr::Instantiation = T->isInstantiationDependentType() | CommonInit->isInstantiationDependentType() | ElementInit->isInstantiationDependentType(). | |
5659 | ! behavior change here, now Expr::TypeDependent = DstType->isDependentType() | SrcExpr->isTypeDependent(). | |
clang/include/clang/AST/ExprCXX.h | ||
2757 | ! behavior change here, now we the ValueDependent, UnexpandedPack takes dimension into account as well. | |
clang/lib/AST/ExprCXX.cpp | ||
444 | ! this change is not trivial, need an review. |
So this patch makes me pretty nervous - it moves a lot of logic, it necessarily changes the way it's structured, the domain is subtle. I wouldn't be terribly surprised if we were missing some tests here.
So this all points to resisting the urge to improve things in this patch, and doing it in followups instead.
clang/include/clang/AST/Expr.h | ||
---|---|---|
154 | This should have some docs like "each concrete expr subclass is expected to compute its dependence and call this in the constructor". | |
4092 | this looks incorrect to me. A conversion's type doesn't depend on the input type. | |
4150 | LG | |
4261 | Are you saying the previous version of this patch didn't match the spec, or didn't match the old behavior, or both? The new one seems pretty different to head. Do these not make a difference for practical cases? | |
5042 | Is this a bugfix? Consider leaving a FIXME instead? | |
5659 | This seems incorrect to me. Like a new-expr, the type of the result doesn't depend on the type of the inputs. | |
clang/include/clang/AST/ExprCXX.h | ||
2757 | this seems like a reasonable/trivial bugfix | |
4120 | Why not have a computeDependence function for this? | |
clang/include/clang/AST/TemplateBase.h | ||
672 | I don't understand this comment, can you expand it? | |
clang/lib/AST/ComputeDependence.cpp | ||
14 | doh, we forgot to fix dependencyflags -> dependenceflags in the previous patch | |
25 | I'm not sure who these comments are for. | |
51 | this is turnTypeToValueDependence(...) | |
55 | and again here | |
166 | apart from any correctness issues, I think this expression is too complicated and should be broken down. | |
281 | might be clearer as turnTypeToValueDependence, I think that's conceptually what's going on | |
286 | this seems redundant, you need the turn OR the ~type | |
364 | I'm not really sure why we're changing this. The new version doesn't seem obviously clearer or faster. Is it a bug fix? | |
462 | nit: if (A) D|= ... ? | |
clang/lib/AST/ExprCXX.cpp | ||
444 | LG |
- address comments
- reverted the bahavior-change changes
- use the turnTypeToValueDependences
clang/include/clang/AST/Expr.h | ||
---|---|---|
4092 | reverted to the old behavior. | |
4261 | ah, you are right, I intended to make the new implementation match the old behavior (not sure what I thought before), updated, it should match the old behavior now. | |
5042 | reverted to old behavior. | |
5659 | sounds fair, reverted to old behavior. | |
clang/include/clang/AST/ExprCXX.h | ||
4120 | this looks like a very straight-forward and simple Dependence initialization, and it doesn't depend on other elements For this particular case, I don't see big difference of having a computeDependence function. If we introduce the error bit, we probably just set it to false' but for CXXFoldExpr example, it makes sense to have a computeDependence function, we should populate the errorbit from its subexprs. and this patches has other places where we directly initialize the dependence, we should have computeDependence for all of them? | |
clang/include/clang/AST/TemplateBase.h | ||
672 | The parameter Deps is the result populated by this method, the caller doesn't need it since we have the computeDependencies. | |
clang/lib/AST/ComputeDependence.cpp | ||
14 | will rename it in a separate patch. | |
25 | These were for code reviews, functions below were in the Expr.h before this patch. I will remove them when landing this patch. | |
55 | this is not exactly the same (by changing the Deps to turnTypeToValueDependence(ArgDeps)), we don't set the value-dependent to Deps if the ArgDeps is value-dependent but not type-dependent. sizeof(unary-expression) is value-dependent if unary-expression is type-dependent, consider the expression sizeof(sizeof(unary-expression)) and unary-expression is type-dependent, the inner sizeof is value-dependent but not type-dependent, and outer sizeof is not value-dependent. | |
286 | oops, this didn't match the old behavior. E is value-dependent if the queried expression is type-dependent. Fixed. | |
364 | not this is not a bug fix, it just moved the implementation from the Expr.cpp to here, are you suggesting to keep it in Expr.cpp? |
(I do think there are ways we could write the dependency computations more clearly, but don't want to diverge further from the existing code)
clang/include/clang/AST/TemplateBase.h | ||
---|---|---|
672 | Thanks, but I meant can you expand the comment :-) | |
clang/lib/AST/ComputeDependence.cpp | ||
172 | the old code for type-dependence is E->getType()->isDependentType(). E->getType() is always the getNonLValueExprType of the E->getWrittenTypeInfo()->getType(), and that function doesn't change the dependence of a type, so I think you can drop the E->getType() here. You still have E->getSubExpr() as a driver for type-dependence, whith I think is incorrect. va_arg(list, t) returns type t regardless of the type of list. |
clang/lib/AST/ComputeDependence.cpp | ||
---|---|---|
607 | I'm impressed this even compiled. With gcc 8.3 I get an error: /work/bf/LLVM/src/clang/lib/AST/ComputeDependence.cpp:607:18: error: use of ‘E’ before deduction of ‘auto’ for (auto *E : E->arguments()) ^ |
clang/lib/AST/ComputeDependence.cpp | ||
---|---|---|
607 | yeah, it was compiled in clang. I saw a failure in the buildbot, fixed d3d844212fc96fb70f77431f0e6a70e0603b0a39. |
This should have some docs like "each concrete expr subclass is expected to compute its dependence and call this in the constructor".