Skip to content

Conversation

ghost
Copy link

@ghost ghost commented Dec 17, 2021

This changes the lint introduced in #7748 to suggest adding a SAFETY comment instead of a Safety comment.

Searching for // Safety: in rust-lang/rust yields 67 results while // SAFETY: yields 1072.
I think it's safe to say that this comment tag is written in upper case, just like TODO, FIXME and so on are. As such I would expect this lint to follow the official convention as well.

Note that I intentionally introduced some casing diversity in tests/ui/undocumented_unsafe_blocks.rs to test more cases than just Safety:.

changelog: Capitalize SAFETY comment in [undocumented_unsafe_blocks]

@rust-highfive
Copy link

r? @giraffate

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties label Dec 17, 2021
@xFrednet
Copy link
Contributor

xFrednet commented Dec 18, 2021

I like standardizing the capitalization. 👍 You might also want to take a look at the missing_safety_doc lint. That one lints missing safety heading in doc comments. 🙃

@ghost
Copy link
Author

ghost commented Dec 18, 2021

@xFrednet I think in that case /// # Safety is fine as-is because it denotes a markdown section like e.g. /// # Examples does (which is not uppercased either). I believe that and this comment tag // SAFETY: which is directly followed by the explanation about why it is safe are two slightly different things.

@giraffate
Copy link
Contributor

@bors r+

Thanks!

@bors
Copy link
Contributor

bors commented Dec 20, 2021

📌 Commit eba4413 has been approved by giraffate

@bors
Copy link
Contributor

bors commented Dec 20, 2021

⌛ Testing commit eba4413 with merge 7905130...

@bors
Copy link
Contributor

bors commented Dec 20, 2021

☀️ Test successful - checks-action_dev_test, checks-action_remark_test, checks-action_test
Approved by: giraffate
Pushing 7905130 to master...

@bors bors merged commit 7905130 into rust-lang:master Dec 20, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants