This is an archive of the discontinued LLVM Phabricator instance.

[AST][FPEnv] Keep FP options in trailing storage of CastExpr
ClosedPublic

Authored by sepavloff on Aug 14 2020, 2:17 AM.

Details

Summary

This change allow a CastExpr to have optional FPOptionsOverride object,
stored in trailing storage. Of all cast nodes only ImplicitCastExpr,
CStyleCastExpr, CXXFunctionalCastExpr and CXXStaticCastExpr are allowed
to have FPOptions.

Diff Detail

Event Timeline

sepavloff created this revision.Aug 14 2020, 2:17 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 14 2020, 2:17 AM
Herald added a subscriber: martong. · View Herald Transcript
sepavloff requested review of this revision.Aug 14 2020, 2:17 AM
martong removed a subscriber: martong.Aug 14 2020, 4:36 AM
kpn added a comment.Aug 14 2020, 12:16 PM

You mentioned in D85920 a need to merge this review with that review. I don't think that's needed. This code here is farther along. It does everything that D85920 does and has necessary pieces implemented as well.

I think this ticket should go into the tree instead of D85920. I'd sign off on it this minute if I had more experience in clang.

In D85960#2218798, @kpn wrote:

You mentioned in D85920 a need to merge this review with that review. I don't think that's needed. This code here is farther along. It does everything that D85920 does and has necessary pieces implemented as well.

I think you are right. I looked through D85920, in essence it does the same changes as this patch. And this patch has better support of serialization and some tests.

Any feedback?

Minor request, but otherwise LGTM.

clang/lib/Analysis/BodyFarm.cpp
187

Can these call sites just be changed to use makeImplicitCast?

This change allow a CallExpr to have optional FPOptionsOverride object,

Should this be CastExpr instead?

stored in trailing storage. The implementaion is made similar to the way
used in CallExpr.

Nit, but that's not really similar. In the CallExpr case the trailing objects are directly managed by CallExpr without using TrailingObjects in the sub-classes.

clang/include/clang/AST/Expr.h
3471

Can you document that hasStoredFPFeatures() must be true as a pre-condition?

3617

Here and elsewhere: numTrailingObjects is not part of the public interface.

clang/include/clang/AST/ExprCXX.h
475–476

/*frob=*/false here and elsewhere.

sepavloff updated this revision to Diff 290487.Sep 8 2020, 8:04 AM

Updated patch

sepavloff edited the summary of this revision. (Show Details)Sep 8 2020, 8:08 AM
sepavloff marked 3 inline comments as done.Sep 8 2020, 8:13 AM

This change allow a CallExpr to have optional FPOptionsOverride object,

Should this be CastExpr instead?

Yes, thank you for the catch.

stored in trailing storage. The implementaion is made similar to the way
used in CallExpr.

Nit, but that's not really similar. In the CallExpr case the trailing objects are directly managed by CallExpr without using TrailingObjects in the sub-classes.

This implementation was obtained by applying changes similar to those made for CallExpr. But you are right, there is some difference in the implementations. I think this statement is not too useful so removed it.

clang/include/clang/AST/Expr.h
3617

Moved these methods to private section.

Follow-up on my previous request. It's fine by me if you commit with that fix.

clang/lib/Analysis/BodyFarm.cpp
187

Can you do the one in makeIntegralCast, too? Thanks.

This revision was not accepted when it landed; it landed in state Needs Review.Sep 12 2020, 12:32 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.