Page MenuHomePhabricator

[CodingStandards] Document coding standard for error and warning messages
ClosedPublic

Authored by jhenderson on Mar 26 2020, 3:06 AM.

Details

Summary

In particular, these messages should start with a lower-case letter and should have no trailing period at the end of the last sentence.

See http://lists.llvm.org/pipermail/llvm-dev/2020-March/140178.html for context.

Diff Detail

Unit TestsFailed

TimeTest
220 msClang.CodeGen::Unknown Unit Message ("")
Script: -- : 'RUN: at line 5'; /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/bin/opt -thinlto-bc -o /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/build/tools/clang/test/CodeGen/Output/thinlto-distributed-newpm.ll.tmp.o /mnt/disks/ssd0/agent/workspace/amd64_debian_testing_clang8/clang/test/CodeGen/thinlto-distributed-newpm.ll

Event Timeline

jhenderson created this revision.Mar 26 2020, 3:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptMar 26 2020, 3:06 AM
aaron.ballman added inline comments.
llvm/docs/CodingStandards.rst
329–330

This advice is not true for diagnostics from the Clang Static Analyzer, where they've chosen to use sentences with capitalization but without punctuation. We should call that style choice out, and clarify that the lowercase/no punctuation style applies to clang as well as clang-tidy.

Mention the static analyzer and other tools explicitly.

Thanks for doing this. The guidance is really helpful to have.

llvm/docs/CodingStandards.rst
330

Suggestion:
[ ... ] without a period if it were to end in one otherwise. For example, "did you forget ';'?" should still end in a question mark.

345

Should "Clang" be capitalized here?

349

I think a comma before "and so on" is appropriate. Alternatively, use "etc." with a comma.

jhenderson marked 4 inline comments as done.

Address review comments.

I think this looks good (with a minor comment) but would like to hear from more people first.

llvm/docs/CodingStandards.rst
331

s/punctions/punctuation/;

aaron.ballman accepted this revision.Mar 26 2020, 9:33 AM

Thanks for working on this! (You should wait to commit for others to weigh in as well.)

llvm/docs/CodingStandards.rst
348

pre-existing -> preexisting

This revision is now accepted and ready to land.Mar 26 2020, 9:33 AM
jhenderson marked 2 inline comments as done.

Address comments. I'll leave this another 24 hours and then commit it, if that's okay?

dblaikie accepted this revision.Mar 30 2020, 12:06 PM

Looks good - thanks!

rnk accepted this revision.Mar 30 2020, 12:26 PM
rnk added inline comments.
llvm/docs/CodingStandards.rst
338

If you look at clang diagnostics, I think most of the time we would use a semicolon to separate sentences here, so it would look like this:

error: file.o: section header 3 is corrupt; size is 10 when it should be 20

However, overall this is useful guidance, and if you want to leave this example as is for migrating existing multi-sentence diagnostics, that's OK with me.

jhenderson marked an inline comment as done.Mar 31 2020, 3:18 AM
jhenderson added inline comments.
llvm/docs/CodingStandards.rst
338

Thanks for the comment. I think there's less clear consensus in the tools on what to do about mid-message ends of sentences. I know that there are several messages following <part1>. <part2> style and also <part1>: <part2> etc. Let's leave this as is for now.

This revision was automatically updated to reflect the committed changes.
dblaikie added inline comments.Apr 1 2020, 1:32 PM
llvm/docs/CodingStandards.rst
338

If there's lack of consensus it might be preferable for this document not to take a stance on the issue? (either drop this bit entirely, or include alternatives equally - especially if clang's convention isn't shown here (& yeah, I find "this is a fragment. This is a sentence" pretty weird/surprising - given how used to clang's conventions I am), since it's the biggest surface area/usage of diagnostics in the project)

jhenderson marked an inline comment as done.Apr 2 2020, 12:26 AM
jhenderson added inline comments.
llvm/docs/CodingStandards.rst
338

The text itself doesn't state how to separate sentences within the messages, and this example wasn't intended to take a stance on how to form such things, but rather to illustrate that it isn't illegal to have full stops elsewhere (based on the principle that there are existing messages in that form). I think that it's also worth noting that nothing in this is intended to prevent specific projects imposing further guidelines, I guess, so I wouldn't encourage people adopting it for new clang diagnostics, I guess.

If somebody wants to go ahead and further try to get consensus on banning the use of full stops at all in diagnostic messages, I'm not going to stand in the way of that (and then this text should definitely be updated). I just don't have a strong motivation/desire to try to do so.

dblaikie added inline comments.Apr 2 2020, 3:02 PM
llvm/docs/CodingStandards.rst
338

Fair enough :)