Page MenuHomePhabricator

[AST] Move dependence computations into a separate file
ClosedPublic

Authored by hokein on Jan 29 2020, 9:16 AM.

Details

Summary

To group the code in one place, simplify it and make it easier to add
the containsErrors bit and find existing bugs.

Diff Detail

Event Timeline

ilya-biryukov created this revision.Jan 29 2020, 9:16 AM
Herald added a reviewer: shafik. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

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

hokein updated this revision to Diff 248439.Mar 5 2020, 4:42 AM
hokein added a subscriber: hokein.
  • rebase to master
  • handle concepts
hokein updated this revision to Diff 248458.Mar 5 2020, 5:58 AM

minor fixes

hokein commandeered this revision.Mar 5 2020, 5:59 AM
hokein added a reviewer: sammccall.
hokein added a reviewer: ilya-biryukov.

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.

bmahjour removed a subscriber: bmahjour.Mar 5 2020, 8:06 AM
martong resigned from this revision.Mar 5 2020, 8:19 AM
nridge added a subscriber: nridge.Thu, Mar 12, 6:54 AM

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.
e.g. parameter packsand instantiation dependence are propagated from type, type dependence is propagated from the subexpr and the writtentypeinfo's type.

Do these not make a difference for practical cases?
(If this is a bug fix, I'd prefer to land it separately)

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?
In particular we'll end up missing haserrors propagation without it, right?

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

hokein updated this revision to Diff 250514.Mon, Mar 16, 4:00 AM
hokein marked 20 inline comments as done.
  • address comments
  • reverted the bahavior-change changes
  • use the turnTypeToValueDependences
hokein added inline comments.Mon, Mar 16, 4:03 AM
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?

hokein updated this revision to Diff 250558.Mon, Mar 16, 7:33 AM
hokein marked 2 inline comments as done.

rebase to master.

sammccall accepted this revision.Mon, Mar 16, 9:31 AM

(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 :-)
(The explanation you just gave is fine)

clang/lib/AST/ComputeDependence.cpp
172

the old code for type-dependence is E->getType()->isDependentType().
The new one is the typedependence of E->getType() | E->getSubExpr() | E->getWrittenTypeInfo()->getType().

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.

This revision is now accepted and ready to land.Mon, Mar 16, 9:31 AM
hokein updated this revision to Diff 250702.Tue, Mar 17, 1:11 AM

Fix the incorrect VAArgExpr dependence.

This revision was automatically updated to reflect the committed changes.
hokein marked an inline comment as done.
dnsampaio added inline comments.
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())
                  ^
hokein marked an inline comment as done.Tue, Mar 17, 2:20 AM
hokein added inline comments.
clang/lib/AST/ComputeDependence.cpp
607

yeah, it was compiled in clang. I saw a failure in the buildbot, fixed d3d844212fc96fb70f77431f0e6a70e0603b0a39.