[cfe-commits] [PATCH] Introduce diagnostics into comment parsing and semantic analysis

[Jordan Rose]

+def warn_doc_html_open_close_mismatch : Warning<
+  "HTML opening tag '%0' closed by '%1'">,
+  InGroup<Doxygen>, DefaultIgnore;

I'm worried about this warning. HTML is not XML (even though I personally prefer XML-style HTML), and so writing this is not at all uncommon:

<ul>
  <li>Item 1
  <li>Item 2
</ul>

Maybe this can be in a subgroup of Doxygen, like -Wdoxygen-html or -Wdoxygen-xhtml. That way it gets turned on by default with -Wdoxygen, but can be turned off as well without turning off everything. (...or -Wdocumentation, whatever the final decision on that is.


+  enum { InvalidParamIndex = ~0U };

If you can declare this earlier, you can use it in the constructor (which is probably a good idea).

+    assert(Index != InvalidParamIndex);

Likewise, this should just call isParamIndexValid(). Encapsulation makes it easier to change things later if necessary.


+  if (!ThisDecl || ThisDecl->getKind() != Decl::Function)
+    Diag(Command->getLocation(),
+         diag::warn_doc_param_not_attached_to_a_function_decl);

This test is wrong; not only will it not catch ObjCMethodDecls (as Doug already mentioned), but it doesn't work for any subclasses of FunctionDecl either. You'll need to use isa<FunctionDecl> || isa<ObjCMethodDecl> to properly handle subclasses.

(ObjCMethodDecl doesn't have any subclasses right now, so if you really don't want to include DeclObjC.h you can get away with using getKind() for that one.)


+  if (Command->getArgCount() != 0) {
+    // Actually, this shouldn't happen because parser will not feed us more
+    // arguments than needed.
+    Diag(Command->getLocation(), diag::warn_doc_command_too_many_arguments)
+      << /* number of arguments */ 2
+      << SourceRange(ArgLocBegin, ArgLocEnd);
+    return Command;
+  }

This is suspicious. If this really can't happen, put it in an assert and fix the parser if it ever fires. If we ever get a new kind of doc comment that has a more generic argument syntax, we can put it back, but right now it's just untestable dead code.