Page MenuHomePhabricator

[Driver, CodeGen] rename options to disable an FP cast optimization
ClosedPublic

Authored by spatel on Apr 29 2018, 8:28 AM.

Details

Summary

As suggested in the post-commit thread for rL331056, we should match these clang options with the established vocabulary of the corresponding sanitizer option. Also, the use of 'strict' is well-known for these kinds of knobs, and we can improve the descriptive text in the docs.

So this intends to match the logic of D46135 but only change the words. I'll post the matching LLVM patch next and link it to this.

Diff Detail

Repository
rL LLVM

Event Timeline

spatel created this revision.Apr 29 2018, 8:28 AM
spatel updated this revision to Diff 144481.Apr 29 2018, 8:31 AM

Patch updated:
I missed a spot in the release notes - forgot to delete 'workaround' when listing the new options.

lebedev.ri accepted this revision.Apr 29 2018, 8:53 AM

Overall makes sense to me.

docs/ReleaseNotes.rst
94 ↗(On Diff #144481)
- in a casted-to integer type
+ in destination integer type

?

96 ↗(On Diff #144481)

s/event/case/ ?

docs/UsersManual.rst
1260 ↗(On Diff #144481)

same

1262 ↗(On Diff #144481)

same

include/clang/Driver/Options.td
1033 ↗(On Diff #144481)

Maybe add HelpText<""> to both of them, just to explain what they are,
specify that the first is the default, and the second relaxes language rules to help broken code.

test/CodeGen/no-junk-ftrunc.c
7 ↗(On Diff #144481)

It wouldn't hurt to duplicate this run-line, and explicitly pass -fno-strict-float-cast-overflow.

This revision is now accepted and ready to land.Apr 29 2018, 8:53 AM
spatel updated this revision to Diff 144487.Apr 29 2018, 9:25 AM
spatel marked 6 inline comments as done.

Patch updated:

  1. Improved language in docs.
  2. Added help text for clang options.
  3. Added driver test with -fstrict-float-cast-overflow specified explicitly.

I'll leave this up for further review in case there are other suggestions.

rsmith added inline comments.Apr 29 2018, 12:46 PM
docs/ReleaseNotes.rst
96 ↗(On Diff #144487)

Add "By default" to the start of this sentence.

spatel updated this revision to Diff 144551.Apr 30 2018, 6:48 AM

Patch updated:
Added "By default" to the description in the user manual and release notes to make it clearer how this behaves.

This revision was automatically updated to reflect the committed changes.
This revision was automatically updated to reflect the committed changes.