On Jun 19, 2012, at 4:36 PM, Dmitri Gribenko <[email protected]> wrote:
> Hi Doug, > > Thank you for the review. Responses inline. Patch attached. > > On Tue, Jun 19, 2012 at 10:47 AM, Douglas Gregor <[email protected]> wrote: >> Index: include/clang/Basic/DiagnosticSemaKinds.td >> =================================================================== >> --- include/clang/Basic/DiagnosticSemaKinds.td (revision 158558) >> +++ include/clang/Basic/DiagnosticSemaKinds.td (working copy) >> @@ -5633,5 +5633,10 @@ >> "definition of %0 must be imported before it is required">; >> } >> >> +let CategoryName = "Comment Issue" in { >> +def warn_not_a_doxygen_after_member_comment : Warning< >> + "not a Doxygen member comment">, InGroup<DiagGroup<"doxygen">>; >> +} // end of comment issue category >> + >> } // end of sema component. >> >> Please define a warning group in DiagnosticGroups.td for Doxygen warnings; I >> suspect we'll have a number of these. >> >> Is this simply a "Doxygen Issue" or "Documentation Issue", rather than a >> "Comment Issue"? > > We can say that it is a "Documentation issue", thus side-stepping the > issue of hardcoding any particular names into clang. Okay! >> + SourceRange Range; >> + CommentKind Kind; >> + bool IsAfterMember; >> + bool IsAlmostAfterMemberComment; >> + StringRef RawText; >> + >> + mutable unsigned BeginLine; ///< Cached line number >> + mutable unsigned EndLine; ///< Cached line number >> + mutable bool BeginLineValid; ///< True if BeginLine is valid >> + mutable bool EndLineValid; ///< True if EndLine is valid >> >> Please pack the various integer/bool/enum data fields into bitfields, >> because the size of RawComment does have an impact on the size of the AST. > > Packed bool and enum fields together, but didn't touch unsigned > members because they are line numbers. Unfortunately, this perfectly valid C++ code: + + CommentKind Kind : 3; + will break MSVC in horrible ways. Please use "unsigned Kind : 3;" here, and add casts where needed. >> + /// \brief Return the Doxygen-style comment attached to a given >> declaration, >> + /// without looking into cache. >> + const RawComment *getRawCommentForDeclNoCache(const Decl *D) const; >> >> It's just a documentation comment, not specifically a Doxygen comment, right? >> >> /** >> + * \brief Given a cursor that represents a declaration, return the >> associated >> + * comment's source range. >> + */ >> +CINDEX_LINKAGE CXSourceRange clang_Cursor_getCommentRange(CXCursor C); >> + >> >> Should this comment say something about this range covering multiple >> adjacent comments? > > Rephrased: > /** > * \brief Given a cursor that represents a declaration, return the associated > * comment's source range. > */ > CINDEX_LINKAGE CXSourceRange clang_Cursor_getCommentRange(CXCursor C); > > /** > * \brief Given a cursor that represents a declaration, return the associated > * comment text, including comment markers. The range may include multiple > * consecutive comments with whitespace in between. > */ > CINDEX_LINKAGE CXString clang_Cursor_getRawCommentText(CXCursor C); The second sentence here should have been added to clang_Cursor_getCommentRange, right? >> + // First check whether we have a comment that comes after a member decl. >> + if (Comment != RawComments.end() && >> + Comment->isDoxygen() && Comment->isAfterMember() && >> + !isa<TagDecl>(D) && !isa<NamespaceDecl>(D)) { >> >> Why the checks against tags and namespaces? Should this also/instead be >> checking isa<TagDecl>(D->getDeclContext()) || >> isa<ObjCContainerDecl>(D->getDeclContext())? > > The reason for this check is that we don't allow TagDecls and > NamespaceDecls to have "after member" ///< comments. (Although -- > just checked -- Doxygen allows them for TagDecls. But that's > confusing to write in source, I don't think it is logical.) Okay, that makes sense. >> + case COMMENTS_RAW_COMMENT: { >> + unsigned Idx = 0; >> + SourceRange SR = ReadSourceRange(F, Record, Idx); >> + bool Merged = Record[Idx++]; >> + Context.addComment(RawComment(SourceMgr, SR, Merged)); >> + break; >> >> This is going to cause us to pull in the contents of each of the files where >> a raw comment occurred, as soon as we load the raw comments from the AST >> file. That's going to be rather expensive, I suspect. Alternatively, we >> could store more in the AST file (including the Kind, IsAfterMember, and >> IsAlmostAfterMemberComment values) and lazily compute RawText only when it's >> actually requested... which should be very rare. > > Done. > >> Also, I think you want to put all of the comments read from the AST file at >> the beginning of the RawCommentList, because right now they could end up >> getting mixed together with comments collecting while parsing the current >> translation unit. This way, the ordering will be correct for precompiled >> headers. (Modules are a separable issue) > > Done. > > Question: > > * "After member comment" sounds awkward (but that's how it is called > once in Doxygen manual). Maybe "trailing comment" is better? Yes, I like "trailing comment" better. This looks great! Please go ahead and commit. - Doug _______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
