Page MenuHomePhabricator

Generalize and harmonize sub-expression traversal
ClosedPublic

Authored by kimgr on Feb 10 2022, 1:01 PM.

Details

Reviewers
aaron.ballman
Summary

CastExpr::getSubExprAsWritten and getConversionFunction used to have
disparate implementations to traverse the sub-expression chain and skip
so-called "implicit temporaries" (which are really implicit nodes added by
Sema to represent semantic details in the AST).

There's some friction in these algorithms that makes it hard to extend and
change them:

  • skipImplicitTemporary is order-dependent; it can skip a CXXBindTemporaryExpr nested inside a MaterializeTemporaryExpr, but not vice versa
  • skipImplicitTemporary only runs one pass, it does not traverse multiple nested sequences of MTE/CBTE/MTE/CBTE, for example

Both of these weaknesses are void at this point, because this kind of
out-of-order multi-level nesting does not exist in the current AST.

Adding a new implicit expression to skip exacerbates the problem, however,
since a node X might show up in any and all locations between the existing.

Thus;

  • Harmonize the form of getSubExprAsWritten and getConversionFunction so they both use a for loop
  • Use the IgnoreExprNodes machinery to skip multiple nodes
  • Rename skipImplicitTemporary to ignoreImplicitSemaNodes to generalize
  • Update ignoreImplicitSemaNodes so it only skips one level per call, to mirror existing Ignore functions and work better with IgnoreExprNodes

This is a functional change, but one without visible effect.

Diff Detail

Event Timeline

kimgr created this revision.Feb 10 2022, 1:01 PM
kimgr requested review of this revision.Feb 10 2022, 1:01 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 10 2022, 1:01 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

@aaron.ballman First refactor to generalize and harmonize getSubExprAsWritten and getConversionFunction. As mentioned in the commit message, this is strictly speaking a functional change, but it should have no visible effect.

kimgr added inline comments.Feb 10 2022, 1:19 PM
clang/lib/AST/Expr.cpp
1903

Typo: IgnoreExpr*e*Nodes

kimgr updated this revision to Diff 411666.Feb 27 2022, 4:25 AM

Fix typo in comment.

kimgr added a comment.Mar 6 2022, 10:54 PM

Gentle weekly ping

Herald added a project: Restricted Project. · View Herald TranscriptMar 6 2022, 10:54 PM
kimgr added a comment.Mar 14 2022, 1:29 PM

@aaron.ballman Friendly Monday ping.

aaron.ballman accepted this revision.Mar 18 2022, 8:01 AM
aaron.ballman added a reviewer: aaron.ballman.

Sorry for the delay in reviewing this, but this looks correct to me. I don't think the precommit CI failures on Debian relate to your patch, either, so this LGTM

kimgr added a comment.Mar 19 2022, 8:15 AM

Thanks! Could you help me land it? Author: Kim Gräsman <kim.grasman at gmail.com>.

aaron.ballman accepted this revision.Mar 21 2022, 12:04 PM

Thanks! Could you help me land it? Author: Kim Gräsman <kim.grasman at gmail.com>.

Happy to!

This revision is now accepted and ready to land.Mar 21 2022, 12:04 PM

Thanks! Could you help me land it? Author: Kim Gräsman <kim.grasman at gmail.com>.

I've commit on your behalf in 276d2143148fd1519af8eef124dabc41eabb1bb0