[PATCH] D61103: [clang] Add tryToAttachCommentsToDecls method to ASTContext

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added a comment.

> The FIXME in tests is fixed now.

... so instead of deleting the test, could you change it to show the current, better diagnostic?



================
Comment at: clang/include/clang/AST/ASTContext.h:818
+  /// For every comment not attached to any decl check if it should be attached
+  /// to any of \param Decls
+  ///
----------------
Please add a period.


================
Comment at: clang/include/clang/AST/ASTContext.h:818
+  /// For every comment not attached to any decl check if it should be attached
+  /// to any of \param Decls
+  ///
----------------
gribozavr wrote:
> Please add a period.
Please add a period.


================
Comment at: clang/lib/AST/ASTContext.cpp:494
+  // Explicitly not calling ExternalSource->ReadComments() as we're not
+  // interested in those.
+  ArrayRef<RawComment *> RawComments = Comments.getComments();
----------------
Would be great to explain why (because we assume that the decls and their comments were parsed just now).  Otherwise the comment could enumerate a lot of other things that we are not calling here either...


================
Comment at: clang/lib/AST/ASTContext.cpp:564
+  for (const Decl *D : Decls) {
+    D = adjustDeclToTemplate(D);
+    if (!CanDeclHaveDocComment(D))
----------------
`getCommentForDecl` checks `D->isInvalidDecl()` first.


================
Comment at: clang/lib/AST/ASTContext.cpp:584
+    // terminating early.
+    for (auto CIt = RawComments.begin(); CIt != RawComments.end(); ++CIt) {
+      RawComment *C = *CIt;
----------------
Scanning all comments for every decl?  Isn't that O(n^2)?

Also logic duplication below.  I was expecting a call to `getRawCommentForDeclNoCache`, with an appropriate flag to disable loading external comments (it is a low-level API, users generally don't call it).


Repository:
  rC Clang

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D61103/new/

https://reviews.llvm.org/D61103