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

[Dmitri Gribenko]

Hi Jordan,

Thank you for review!

On Wed, Jul 11, 2012 at 9:38 AM, Jordan Rose <jordan_rose at apple.com> wrote:
> +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>

bool Sema::HTMLOpenTagNeedsClosing(StringRef Name)
returns false for such tags.  It allows such tags to have closing
tags, but suppresses the warning if closing tag is omitted. Added 'li'
to the list, thanks!

Also added -Wdocumentation-html flag for semantic HTML errors to allow
the user to disable only this warning.

> +  enum { InvalidParamIndex = ~0U };
>
> If you can declare this earlier, you can use it in the constructor (which is probably a good idea).

Done.

> +    assert(Index != InvalidParamIndex);
>
> Likewise, this should just call isParamIndexValid(). Encapsulation makes it easier to change things later if necessary.

Done.

> +  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.)

Done, but please look at the tests (test/Sema/warn-documentation.m).
Anything else worth adding?

> +  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.

Done.

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>*/