Adds a warning when an inline Doxygen comment has no argument

[Mordante]

Summary

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

where \em has no argument

This warning is enabled with the -Wdocumentation option.

[gribozavr]

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]

Addresses @gribozavr comments.

[Mordante]

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]

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

[Mordante]

Thanks for the review.

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

[davezarzycki]

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

/// \c @foobar

[Mordante]

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

[davezarzycki]

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

[Mordante]

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?

[davezarzycki]

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.

[Mordante]

I created D66700 with an improved warning message.

cfe/trunk/include/clang/Basic/DiagnosticCommentKinds.td

 def warn_doc_deprecated_not_sync : Warning<
   "declaration is marked with '\\deprecated' command but does not have "
   "a deprecation attribute">,
   InGroup<DocumentationDeprecatedSync>, DefaultIgnore;
 
 def note_add_deprecation_attr : Note<
   "add a deprecation attribute to the declaration to silence this warning">;
 
+// inline contents commands
+
+def warn_doc_inline_contents_no_argument : Warning<
+  "'%select{\\|@}0%1' command does not have an argument">,
+  InGroup<Documentation>, DefaultIgnore;
+
 // verbatim block commands
 
 def warn_verbatim_block_end_without_start : Warning<
   "'%select{\\|@}0%1' command does not terminate a verbatim text block">,
   InGroup<Documentation>, DefaultIgnore;
 
 def warn_unknown_comment_command_name : Warning<
   "unknown command tag name">,
   InGroup<DocumentationUnknownCommand>, DefaultIgnore;
 
 def warn_correct_comment_command_name : Warning<
   "unknown command tag name '%0'; did you mean '%1'?">,
   InGroup<DocumentationUnknownCommand>, DefaultIgnore;
 
 } // end of documentation issue category
 } // end of AST component

cfe/trunk/lib/AST/CommentParser.cpp

IC = S.actOnInlineCommand(CommandTok.getLocation(),
                               CommandTok.getCommandID(),
                               ArgTok.getLocation(),
                               ArgTok.getEndLocation(),
                               ArgTok.getText());
   } else {
     IC = S.actOnInlineCommand(CommandTok.getLocation(),
                               CommandTok.getEndLocation(),
                               CommandTok.getCommandID());
+
+    Diag(CommandTok.getEndLocation().getLocWithOffset(1),
+         diag::warn_doc_inline_contents_no_argument)
+        << CommandTok.is(tok::at_command)
+        << Traits.getCommandInfo(CommandTok.getCommandID())->Name
+        << SourceRange(CommandTok.getLocation(), CommandTok.getEndLocation());
   }
 
   Retokenizer.putBackLeftoverTokens();
 
   return IC;
 }
 
 HTMLStartTagComment *Parser::parseHTMLStartTag() {

cfe/trunk/test/Sema/warn-documentation.cpp

struct test_attach38 {
   template <typename B>
   void test_attach39(T a, B b);
 };
 
 template <>
 template <typename B>
 void test_attach38<int>::test_attach39(int, B);
 
+// The inline comments expect a string after the command.
+// expected-warning@+1 {{'\a' command does not have an argument}}
+/// \a
+int test_inline_no_argument_a_bad(int);
+
+/// \a A
+int test_inline_no_argument_a_good(int);
+
+// expected-warning@+1 {{'@b' command does not have an argument}}
+/// @b
+int test_inline_no_argument_b_bad(int);
+
+/// @b A
+int test_inline_no_argument_b_good(int);
+
+// expected-warning@+1 {{'\c' command does not have an argument}}
+/// \c
+int test_inline_no_argument_c_bad(int);
+
+/// \c A
+int test_inline_no_argument_c_good(int);
+
+// expected-warning@+1 {{'\e' command does not have an argument}}
+/// \e
+int test_inline_no_argument_e_bad(int);
+
+/// \e A
+int test_inline_no_argument_e_good(int);
+
+// expected-warning@+1 {{'\em' command does not have an argument}}
+/// \em
+int test_inline_no_argument_em_bad(int);
+
+/// \em A
+int test_inline_no_argument_em_good(int);
+
+// expected-warning@+1 {{'\p' command does not have an argument}}
+/// \p
+int test_inline_no_argument_p_bad(int);
+
+/// \p A
+int test_inline_no_argument_p_good(int);
 
 // PR13411, reduced.  We used to crash on this.
 /**
  * @code Aaa.
  */
 void test_nocrash1(int);
 
 // We used to crash on this.