This is an archive of the discontinued LLVM Phabricator instance.

[clang] Small improvments after Adding APValue to ConstantExpr
ClosedPublic

Authored by Tyker on Jun 15 2019, 6:52 AM.

Details

Summary

this patch has multiple small improvements related to the APValue in ConstantExpr.

changes:

  • APValue in ConstantExpr are now cleaned up using ASTContext::addDestruction instead of there own system.
  • ConstantExprBits Stores the ValueKind of the result beaing stored.
  • VerifyIntegerConstantExpression now stores the evaluated value in ConstantExpr.
  • the Constant Evaluator uses the stored value of ConstantExpr when available.

Diff Detail

Event Timeline

Tyker created this revision.Jun 15 2019, 6:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 15 2019, 6:52 AM
Herald added a subscriber: cfe-commits. · View Herald Transcript
Tyker updated this revision to Diff 204916.Jun 15 2019, 6:59 AM
Tyker updated this revision to Diff 204992.Jun 17 2019, 12:26 AM
rsmith accepted this revision.Jun 17 2019, 11:52 AM

Nice cleanup!

clang/include/clang/Sema/Sema.h
10298 ↗(On Diff #204992)

Do you need this new flag? No callers are passing it.

This revision is now accepted and ready to land.Jun 17 2019, 11:52 AM
Tyker marked an inline comment as done.Jun 19 2019, 3:47 AM
Tyker added inline comments.
clang/include/clang/Sema/Sema.h
10298 ↗(On Diff #204992)

the idea behind it is that not all integral constant expression benefit from storing the result. i have not analyzed which benefit from it and which don't. adding a FIXME would have probably be more appropriate.

rsmith added inline comments.Jun 20 2019, 10:28 AM
clang/include/clang/Sema/Sema.h
10298 ↗(On Diff #204992)

It's useful to have a consistent AST representation, so I'd be inclined to at least always create a ConstantExpr node. Also, once we start allowing consteval evaluations that have side effects (for example, reflection-related actions that modify the AST), I think we'll need to always store the result to ensure we never evaluate a constant expression twice (and trigger its side-effects to occur twice).

Tyker updated this revision to Diff 205944.Jun 20 2019, 11:05 PM
Tyker marked an inline comment as done.

made the requested changes.

clang/include/clang/Sema/Sema.h
10298 ↗(On Diff #204992)

I removed the StoreResult flag.
evaluating expression only once is going to be mandatory in the future but i think it is hard to reach, semantic checking often needs intermediate values. so we will need probably need to integrate semantic checking to the expression evaluator to use its intermediate value. and do so without code duplication with the non-constant context checking.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2019, 1:24 AM