Page MenuHomePhabricator

Adds a warning when an inline Doxygen comment has no argument
ClosedPublic

Authored by Mordante on Jul 13 2019, 9:04 AM.

Details

Summary

It warns for for comments like
/** \pre \em */

where \em has no argument

This warning is enabled with the -Wdocumentation option.

Diff Detail

Repository
rL LLVM

Event Timeline

Mordante created this revision.Jul 13 2019, 9:04 AM

Is it true that all inline commands require an argument?

For example, "\&" does not.

clang/test/Sema/warn-documentation.cpp
1030 ↗(On Diff #209700)

Please move this new section below the test_attach tests.

1033 ↗(On Diff #209700)

Please un-abbreviate b and g into bad and good.

Mordante updated this revision to Diff 211340.Jul 23 2019, 11:29 AM

Addresses @gribozavr comments.

Mordante marked 2 inline comments as done.Jul 23 2019, 11:31 AM

All inline commands defined in include/clang/AST/CommentCommands.td require an argument. The escape commands, like \&, are handled in the switch starting at lib/AST/CommentLexer.cpp:366. They are stored as Text and not as an InlineCommand.

If you want I can add an extra field in the Command class in include/clang/AST/CommentCommands.td. Something like bit IsEmptyInlineCommandAllowed = 0;.

gribozavr accepted this revision.Jul 29 2019, 5:30 AM

Thank you! Do you have commit access or would you like me to commit this patch for you?

This revision is now accepted and ready to land.Jul 29 2019, 5:30 AM

Thanks for the review.

I don't have commit access. So yes please commit the patch for me.

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptAug 5 2019, 1:04 AM

Hello – This change seems to have exposed a bug in -Wdocumentation argument parsing. For example, this warns when it shouldn't(?):

/// \c @foobar

I think it should warn; according to the documentation [1] \c expects a word. Testing with Doxygen indeed gives a warning.

Can you post the real comment where this occurs?

[1] http://www.doxygen.nl/manual/commands.html#cmdc

From the Swift language source (http://github.com/apple/swift):

include/swift/AST/Attr.h:/ A limited variant of \c @objc that's used for classes with generic ancestry.
include/swift/AST/Decl.h:
/ \c @usableFromInline attribute are treated as public. This is normally
lib/Sema/LookupVisibleDecls.cpp:/// If \c baseType is \c @dynamicMemberLookup, this looks up any keypath

Unfortunately I'm not able to quickly find the Doxygen output of Swift online. When I process:
A limited variant of \c @objc that's used for classes with generic ancestry.
with Doxygen I get:
A limited variant of that's used for classes with generic ancestry.
Since the @ is used for commands I think they should be escaped like:
include/swift/AST/Decl.h: /// such as \c \@testable and \c \@usableFromInline into account.

Can you verify whether the Doxygen output for the Swift documentation is correct without escaping the \c @foo ? Or do you have a link to the online version?

I don't think anybody is running doxygen over the Swift source right now, so I'll just escape the @ sign. That being said, can we improve this diagnostic? "command does not have an argument" is confusing when a given command does have an argument, just a malformed one. What do you think about "command must precede a word"? At least that gives a hint that the argument is not a "word" according to doxygen.