This is an archive of the discontinued LLVM Phabricator instance.

Clarify when fix-it hints on warnings are appropriate
ClosedPublic

Authored by aaronpuchert on May 26 2019, 2:37 PM.

Details

Summary

This is not a change in the rules, it's meant as a clarification about
warnings. Since the recovery from warnings is a no-op, the fix-it hints
on warnings shouldn't change anything. Anything that doesn't just
suppress the warning and changes the meaning of the code (even if it's
for the better) should be on an additional note.

Diff Detail

Repository
rL LLVM

Event Timeline

aaronpuchert created this revision.May 26 2019, 2:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 26 2019, 2:37 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript

Feel free to suggest better wording, I'm not a native speaker.

Thank you for working on this!

docs/InternalsManual.rst
426–428 ↗(On Diff #201469)

How about:

Fix-it hints on  a warning must not change the meaning of code, only clarify it. For example, adding parentheses in case of counterintuitive precedence to clarify the order of operations.
aaronpuchert marked 2 inline comments as done.May 27 2019, 12:07 PM
aaronpuchert added inline comments.
docs/InternalsManual.rst
426–428 ↗(On Diff #201469)

must not change the meaning of code, only clarify it

Wasn't sure about the order between “not change”/“only clarify”. If it sounds better to you, I'll take that.

For example, adding parentheses in case of counterintuitive precedence to clarify the order of operations.

Can we add a finite verb, like “For example, it would be fine to add parantheses ...”

Also, I would probably shorten “in case of counterintuitive precedence to clarify the order of operations” to “to clarify the precedence of operators.” So we have

For example, it would be fine to add parantheses to clarify the precedence of operators.

aaron.ballman added inline comments.May 27 2019, 12:21 PM
docs/InternalsManual.rst
426–428 ↗(On Diff #201469)

Wasn't sure about the order between “not change”/“only clarify”. If it sounds better to you, I'll take that.

I think the most important bit is that it must not change code meaning, so that's why I suggest putting it first.

For example, it would be fine to add parantheses to clarify the precedence of operators.

I like it! Though I'd spell it parentheses instead. ;-)

aaronpuchert marked 3 inline comments as done.May 27 2019, 12:33 PM
aaronpuchert added inline comments.
docs/InternalsManual.rst
426–428 ↗(On Diff #201469)

Though I'd spell it parentheses instead. ;-)

There must have been a red squiggly line, but I've apparently decided to ignore it.

aaronpuchert marked an inline comment as done.

Put the important part first, and the rest in a separate sentence.

Thanks, looks good to me.

This revision is now accepted and ready to land.May 27 2019, 6:11 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 31 2019, 2:25 PM