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

[Dmitri Gribenko]

On Tue, Jul 10, 2012 at 1:43 PM, Douglas Gregor <dgregor at apple.com> wrote:
> Index: include/clang/Basic/DiagnosticGroups.td
> ===================================================================
> --- include/clang/Basic/DiagnosticGroups.td     (revision 159963)
> +++ include/clang/Basic/DiagnosticGroups.td     (working copy)
> @@ -58,6 +58,7 @@
>  def : DiagGroup<"discard-qual">;
>  def : DiagGroup<"div-by-zero">;
>  def Doxygen : DiagGroup<"doxygen">;
> +def DoxygenPedantic : DiagGroup<"doxygen-pedantic">;
>  def EmptyBody : DiagGroup<"empty-body">;
>  def ExtraTokens : DiagGroup<"extra-tokens">;
>
> Should "doxygen" and "doxygen-pedantic" be "documentation" and "documentation-pedantic"? Some of these warnings (e.g., parameter not found) apply equally to non-Doxygen documentation systems.

Done.

> @@ -558,6 +581,14 @@
>
>    static bool classof(const ParamCommandComment *) { return true; }
>
> +  enum PassDirection {
> +    In,
> +    Out,
> +    InOut
> +  };
> +
> +  static const char *directionToString(PassDirection D);
> +
>
> getDirectionAsString(), perhaps? Function names should generally start with verbs.

Done.

> +  const FunctionDecl *FD = dyn_cast_or_null<FunctionDecl>(ThisDecl);
> +  // We already warned that this \\param is not attached to a function decl.
> +  if (!FD)
> +    return Command;
>
> Can we make this work for ObjCMethodDecls, too? This can go into a follow-up patch.

Done.

> As a stretch goal, one could imagine that \param might make sense for typedefs of function types, function pointer types, or block pointer types (although that's not nearly as important as ObjCMethodDecls).

I agree it will make sense.

> +    if (OpenLineInvalid || CloseLineInvalid || OpenLine == CloseLine)
> +      Diag(HOT->getLocation(), diag::warn_doc_html_open_close_mismatch)
> +        << HOT->getTagName() << HCT->getTagName()
> +        << HOT->getSourceRange() << HCT->getSourceRange();
> +    else {
> +      Diag(HOT->getLocation(), diag::warn_doc_html_open_close_mismatch)
> +        << HOT->getTagName() << HCT->getTagName()
> +        << HOT->getSourceRange();
> +      Diag(HCT->getLocation(), diag::note_doc_html_closing_tag)
> +        << HCT->getSourceRange();
> +    }
>
> And here I thought we weren't parsing HTML ;)  (I do agree that it makes sense to diagnose these, since we're here anyway. The client can handle recovery and rendering).

We're not really parsing, just checking.  Per Jordan's comment I moved
this warning under a separate flag -Wdocumentation-html which is part
of -Wdocumentation, so that the user can disable this warning
selectively if they want.

> +  // Don't parse the comment if Doxygen diagnostics are ignored.
> +  if (NumDecls != 0 && Group[0] &&
> +      Diags.getDiagnosticLevel(diag::warn_doc_param_not_found,
> +                               Group[0]->getLocation())
> +        != DiagnosticsEngine::Ignored) {
> +    // See if there are any new comments that are not attached to a decl.
> +    ArrayRef<RawComment *> Comments = Context.getRawCommentList().getComments();
> +    if (!Comments.empty() &&
> +        !Comments.back()->isAttached()) {
> +      // There is at least one comment that not attached to a decl.
> +      // Maybe it should be attached to one of these decls?
> +      //
> +      // Note that this way we pick up not only comments that precede the
> +      // declaration, but also comments that *follow* the declaration -- thanks to
> +      // the lookahead in the lexer: we've consumed the semicolon and looked
> +      // ahead through comments.
> +      for (unsigned i = 0; i != NumDecls; ++i)
> +        Context.getCommentForDecl(Group[i]);
> +    }
> +  }
> +
>
> Very nice!

Per our discussion, I added some special-casing for typedefs.  I don't
know if any other special cases are required.  Also I found that we
need to handle TagDecls, namespaces and ObjC @interfaces separately --
because these are not part of any decl group.  Please look at ObjC
tests in test/Sema/warn-documentation.m -- anything worth adding to
it?

> As a performance optimization, should we keep track of the last attached comment, so that we can limit the search that getCommentForDecl does to the range [last attached + 1, end()]? It seems unfortunate that the lower_bound in getRawCommentForDeclNoCache() always starts at the beginning of the list of raw comments, when we could easily tell it to search a restricted range.

Per our discussion, this is broken by:

/// Aaa
typedef struct S { /** Bbb */ int a; } S;

... but we can still check if the last not attached comment is the one
that would be found by lower_bound.  If not -- call lower_bound.  Will
try doing that as followup.

> Aside: should getCommentForDecl() learn that the documentation for ParmVarDecls is actually attached to the part FunctionDecl (or ObjCMethodDecl), and get the documentation from there?

For generality of getCommentForDecl(), I think it should do that.  But
I think that most clients will want to poke the FullComment
themselves.  Should I implement this?

Committed r160078.

Dmitri

-- 
main(i,j){for(i=2;;i++){for(j=2;j<i;j++){if(!(i%j)){j=0;break;}}if
(j){printf("%d\n",i);}}} /*Dmitri Gribenko <gribozavr at gmail.com>*/