Hi Jordan, Thank you for review!
On Wed, Jul 11, 2012 at 9:38 AM, Jordan Rose <[email protected]> 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 <[email protected]>*/ _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
