Page MenuHomePhabricator

[AST] Factor out the logic of the various Expr::Ignore*
ClosedPublic

Authored by riccibruno on Jan 25 2019, 4:30 PM.

Details

Summary

Now that the implementation of all of the Expr::Ignore* is in Expr.cpp we can try to remove some duplication. Do this by separating the logic of the Expr::Ignore* from the iterative loop.

This is NFC, except for one change: IgnoreParenImpCasts now skips, among other things, everything that IgnoreImpCasts skips. This means FullExpr are now skipped by IgnoreParenImpCasts. This was likely an oversight when FullExpr was added to the nodes skipped by IgnoreImpCasts.

Diff Detail

Repository
rC Clang

Event Timeline

riccibruno created this revision.Jan 25 2019, 4:30 PM

Update the comment in IgnoreImpCastsExtraSingleStep and return early from IgnoreImpCastsExtraSingleStep and IgnoreImplicitSingleStep when IgnoreImpCastsSingleStep skipped something.

Adding Richard for opinions on whether IgnoreParenImpCasts should skip full expressions, and also the changes to accepting null expressions as arguments in more places.

lib/AST/Expr.cpp
2559–2582

I'd remove this comment.

2562

Do we really need to accept null arguments? We didn't previously, and this increases the overhead of something we do pretty often (though it's probably only a negligible performance hit).

2710–2711

I'd remove this comment as well.

2717

I wonder if it would make more sense to use a parameter pack here so that this can be expanded to an arbitrary combination of single steps? The inner part of the while loop would look something like:

template <typename ...Func>
Expr *IgnoreExprNodes(Expr *E, Func&& ...Fns) {
  ...
  while (E != LastE) {
    LastE = E;
    for (auto &&Fn : {Fns...})
      E = std::forward<decltype(Fn)>(Fn)(E);
  }
  ...
}
riccibruno edited the summary of this revision. (Show Details)Jan 28 2019, 7:19 AM
riccibruno marked 4 inline comments as done.

Removed the separator comments in Expr.cpp
Mode IgnoreExprNodes work with an arbitrary number of functions.

lib/AST/Expr.cpp
2562

I did benchmark it to see if there is any difference but could not measure any (looking at the generated code this adds a single null check at the start). My concern was that it could be possible that some AST node has a null child, perhaps when the AST is malformed. I will however defer to you or Richard since I probably lack some of the background needed to make this choice.

2717

I like it! Thanks for the suggestion. This particular solution did not work because it requires I believe all the types to be equal for the auto deduction to succeed. However a recursive solution works fine. I looked quickly at the generated code and it is identical as far as I can tell. Let me know if there are issues with it.

aaron.ballman added inline comments.Jan 30 2019, 6:07 AM
lib/AST/Expr.cpp
2562

If it doesn't seem to have a measurable performance impact, I think it's reasonable.

2563

No else after a return

2580

No else after a return (same elsewhere in the patch).

2684

Rather than an unnamed namespace, you should prefer static function declarations per the style guide.

2695

Formatting looks off here, did clang-format produce this?

riccibruno updated this revision to Diff 184944.Feb 3 2019, 6:02 AM
riccibruno marked 6 inline comments as done.

Rebased and addressed Aaron's comments, except for the null check issue on which I am still undecided.

lib/AST/Expr.cpp
2562

It is something that is actually possible to audit. I did look at where each of the skipped node are created and it *seems* that a null child is not possible. However it is very easy to miss a case and adding the null check give the various Expr::Ignore* the following nice property:

Regardless of the state of the AST, for every expression E (null or not). Ignore*(E) produces the correct result (which may be null).

But perhaps the correct invariant to maintain is that these nodes have always a non-null child ?

I re-ran the benchmarks more carefully and here are the results I got (on my usual benchmark: -fyntax-only on all of Boost, 20 iterations with perf stat):

8.2117 +- 0.0131 seconds (with the null check)
8.2028 +- 0.0058 seconds (without the null check)

2695

It did, though with the static specifier clang-format adds a new line.

aaron.ballman added inline comments.Feb 5 2019, 11:03 AM
lib/AST/Expr.cpp
2562

But perhaps the correct invariant to maintain is that these nodes have always a non-null child ?

I believe we treat this as an invariant in a lot of other places, so it seems like a reasonable invariant here. Being more resilient is not a bad thing, but it may also mask broken invariants elsewhere.

I'll leave it to @rsmith to make the final call though.

riccibruno marked an inline comment as done.Feb 5 2019, 2:03 PM
riccibruno added inline comments.
lib/AST/Expr.cpp
2562

All right. Lets go with dyn_cast to preserve the original behavior.

riccibruno updated this revision to Diff 185539.Feb 6 2019, 6:23 AM

Go back to using dyn_casts.

aaron.ballman accepted this revision.Feb 6 2019, 11:24 AM

LGTM! You should give @rsmith a few days in case he wants to express an opinion though.

lib/AST/Expr.cpp
2731–2733

Formatting (same elsewhere).

This revision is now accepted and ready to land.Feb 6 2019, 11:24 AM

@void @rnk Perhaps you can comment on this: currently Expr::IgnoreImpCasts skips FullExprs, but Expr::IgnoreParenImpCasts only skips (via IgnoreParens) ConstantExprs. Is there any reason for this inconsistency ? I would like to add FullExpr to the nodes skipped by IgnoreParenImpCasts for consistency but I am worried about unexpected issues even though all tests pass.

void added a comment.Feb 8 2019, 12:22 PM

@void @rnk Perhaps you can comment on this: currently Expr::IgnoreImpCasts skips FullExprs, but Expr::IgnoreParenImpCasts only skips (via IgnoreParens) ConstantExprs. Is there any reason for this inconsistency ? I would like to add FullExpr to the nodes skipped by IgnoreParenImpCasts for consistency but I am worried about unexpected issues even though all tests pass.

I don't think there was an explicit reason beyond "I didn't need to do it at the time". So probably just an oversight on my part. I don't know the code nearly as well as @rnk, so I could be wrong, but I think the existing tests should tell you if something went haywire if you skip FullExprs.

rnk added a comment.Feb 8 2019, 12:34 PM

@void @rnk Perhaps you can comment on this: currently Expr::IgnoreImpCasts skips FullExprs, but Expr::IgnoreParenImpCasts only skips (via IgnoreParens) ConstantExprs. Is there any reason for this inconsistency ? I would like to add FullExpr to the nodes skipped by IgnoreParenImpCasts for consistency but I am worried about unexpected issues even though all tests pass.

Yes, @rsmith asked me to skip all FullExprs, but that change did not pass tests, so I only made IgnoreParens ignore ConstantExpr to preserve existing behavior. There is no good design reason for it to be that way, and if you can adjust the callers to account for the new behavior, I think making them consistent would be good.

I don't think there was an explicit reason beyond "I didn't need to do it at the time". So probably just an oversight on my part. I don't know the code nearly as well as @rnk, so I could be wrong, but I think the existing tests should tell you if something went haywire if you skip FullExprs.

I see. I am a bit weary about relying on test coverage to validate a change. It is certainly a necessary condition but I am not sure that it is a sufficient condition.

Yes, @rsmith asked me to skip all FullExprs, but that change did not pass tests, so I only made IgnoreParens ignore ConstantExpr to preserve existing behavior. There is no good design reason for it to be that way, and if you can adjust the callers to account for the new behavior, I think making them consistent would be good.

I agree, but for now what I would like to do is just make sure that IgnoreParenImpCasts skips, among other things, every node skipped by IgnoreImpCasts. Not doing so is highly misleading given the names. (An other fun one is that you would think that IgnoreParenImpCasts() = IgnoreParens() + IgnoreImpCasts() but sadly that's not the case since IgnoreParenImpCasts() skips additionally MaterializeTemporaryExpr and SubstNonTypeTemplateParmExpr, even though IgnoreParenCasts() = IgnoreParens() + IgnoreCasts()).

I don't think there was an explicit reason beyond "I didn't need to do it at the time". So probably just an oversight on my part. I don't know the code nearly as well as @rnk, so I could be wrong, but I think the existing tests should tell you if something went haywire if you skip FullExprs.

I see. I am a bit weary about relying on test coverage to validate a change. It is certainly a necessary condition but I am not sure that it is a sufficient condition.

Yes, @rsmith asked me to skip all FullExprs, but that change did not pass tests, so I only made IgnoreParens ignore ConstantExpr to preserve existing behavior. There is no good design reason for it to be that way, and if you can adjust the callers to account for the new behavior, I think making them consistent would be good.

I agree, but for now what I would like to do is just make sure that IgnoreParenImpCasts skips, among other things, every node skipped by IgnoreImpCasts. Not doing so is highly misleading given the names. (An other fun one is that you would think that IgnoreParenImpCasts() = IgnoreParens() + IgnoreImpCasts() but sadly that's not the case since IgnoreParenImpCasts() skips additionally MaterializeTemporaryExpr and SubstNonTypeTemplateParmExpr, even though IgnoreParenCasts() = IgnoreParens() + IgnoreCasts()).

FWIW, I was rather disappointed in a recent review to learn that IgnoreParens() means "ignore parens... and a whole bunch of other stuff like generic selection expressions".

FWIW, I was rather disappointed in a recent review to learn that IgnoreParens() means "ignore parens... and a whole bunch of other stuff like generic selection expressions".

+1 Indeed.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 17 2019, 5:33 AM