This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Don't persist invalid feature state on .option arch error
ClosedPublic

Authored by jrtc27 on Jun 6 2023, 7:34 AM.

Details

Summary

Otherwise subsequent .option arch, +foo directives (but not -, since
those have their own separate validation) fail the parseFeatureBits
check, leading to cascading errors.

Diff Detail

Event Timeline

jrtc27 created this revision.Jun 6 2023, 7:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 6 2023, 7:34 AM
jrtc27 requested review of this revision.Jun 6 2023, 7:34 AM
This revision is now accepted and ready to land.Jun 6 2023, 7:37 AM
MaskRay accepted this revision.Jun 6 2023, 7:40 AM
MaskRay added inline comments.
llvm/test/MC/RISCV/option-invalid.s
50
jrtc27 updated this revision to Diff 528864.Jun 6 2023, 7:45 AM

Use --implicit-check-not for a more general approach

jrtc27 added inline comments.Jun 6 2023, 7:47 AM
llvm/test/MC/RISCV/option-invalid.s
50

Short comments don't normally get full stops?

MaskRay added inline comments.Jun 6 2023, 7:52 AM
llvm/test/MC/RISCV/option-invalid.s
50

In comments, a complete sentence gets a full stop. An incomplete one doesn't.
There may be cases where a short complete sentence doesn't get a full stop, but I think we don't encourage that:)

(
As a difference, diagnostic messages typically do not get full stops
https://llvm.org/docs/CodingStandards.html#error-and-warning-messages
)

jrtc27 added inline comments.Jun 6 2023, 7:57 AM
llvm/test/MC/RISCV/option-invalid.s
50

Like many comments I view this more as a sentence fragment that happens to technically be a complete sentence. Adding a full stop changes the tone of the comment in a way that isn't appropriate IMO. I'm not fundamentally opposed to it, it just feels quite unnatural to do.

MaskRay added inline comments.Jun 6 2023, 8:45 AM
llvm/test/MC/RISCV/option-invalid.s
50

OK, I don't actually mind much about the comment style in the tests. It was just a suggestion. Feel free to land the patch as is:)