Page MenuHomePhabricator

Inform the AST of #pragma FENV_ACCESS use
ClosedPublic

Authored by kpn on Jul 26 2018, 10:07 AM.

Details

Summary

We have in place support for parsing #pragma FENV_ACCESS, but that information is then discarded with a warning to the user that we don't support it.

This patch gets us one step closer by getting the info down into the AST.

Diff Detail

Repository
rL LLVM

Event Timeline

kpn created this revision.Jul 26 2018, 10:07 AM

Thanks, this is definitely a step in the right direction.

include/clang/AST/Expr.h
3257–3261 ↗(On Diff #157517)

Nit: I think this should be capitalized as FEnvAccess. Lowercasing the "ccess" of "access" but not the "nv" of "environment" seems inconsistent to me, and falsely makes "FENV" look like an initialism or acronym.

include/clang/Basic/LangOptions.h
273–280 ↗(On Diff #157517)

These constructors need to be updated.

lib/Parse/ParsePragma.cpp
619–623 ↗(On Diff #157517)

You need to do *something* in this case. Currently, FPC is read uninitialized a few lines below when this happens. How about just treating this as the same as OFF for now, since that is our default.

lib/Parse/ParseStmt.cpp
353 ↗(On Diff #157517)

Delete this line.

lib/Sema/SemaAttr.cpp
779 ↗(On Diff #157517)

Not directly related to your patch, but... treating FPFeatures as persistent Sema state will be error-prone. For example, we'll get the wrong features in template instantiation and implicitly-generated special member functions. But I don't have a good alternative to suggest right now (other than tracking down the places where we need to save and restore this extra state and doing so), so this is just a concern, not a call to action at this point.

kpn updated this revision to Diff 158036.Jul 30 2018, 12:04 PM

Updated based on review.

kpn marked 3 inline comments as done.Jul 30 2018, 12:05 PM
kpn added inline comments.
include/clang/Basic/LangOptions.h
273–280 ↗(On Diff #157517)

I think this is what you intend?

rsmith accepted this revision.Jul 30 2018, 12:56 PM
rsmith added inline comments.
lib/Parse/ParsePragma.cpp
619–623 ↗(On Diff #157517)

Drop the #if NOTYET part; we don't like having checked-in but commented-out code. Just keep the FIXME comment.

109 ↗(On Diff #158036)

We also need to pass the pragma on to Sema when it's set to OFF.

This revision is now accepted and ready to land.Jul 30 2018, 12:56 PM
kpn marked an inline comment as done.Jul 31 2018, 5:50 AM
kpn updated this revision to Diff 158229.Jul 31 2018, 6:00 AM

Thanks for the fast turnaround! I don't have commit access, can I get you to commit it for me? And can I get you to put my email address <kevin.neal@sas.com> in the commit message?

I'll start looking at Sema like you said.

kpn updated this revision to Diff 158307.Jul 31 2018, 9:44 AM

Replace accidentally lost comment.

kpn updated this revision to Diff 158351.Jul 31 2018, 11:35 AM
kpn marked an inline comment as done.

This includes the change to pass the annotation token when FENV_ACCESS is turned off.

Closed by commit rL339693 (authored by kpn, committed by ). · Explain WhyAug 14 2018, 10:07 AM
This revision was automatically updated to reflect the committed changes.