[PATCH] D60432: [clang][ASTContext] Simplify caching for declaration-related comments

[Dmitri Gribenko via Phabricator via cfe-commits]

gribozavr added inline comments.


================
Comment at: clang/include/clang/AST/ASTContext.h:728
 
-  class RawCommentAndCacheFlags {
+  class CommentAndOrigin {
   public:
----------------
`RawCommentAndOrigin`?


================
Comment at: clang/include/clang/AST/ASTContext.h:751
 
-  /// Mapping from declarations to comments attached to any
+  /// Mapping from canonical declarations to comments attached to any
   /// redeclaration.
----------------
Please remove "canonical", it is not correct according to the implementation.

Please also update the patch description that currently says "I assume we can cache comments for canonical declarations only, not for every redeclaration."


================
Comment at: clang/lib/AST/ASTContext.cpp:372
   D = adjustDeclToTemplate(D);
+  const Decl* CanonicalDecl = D->getCanonicalDecl();
 
----------------
jkorous wrote:
> gribozavr wrote:
> > jkorous wrote:
> > > arphaman wrote:
> > > > Why are we now checking for the canonical declaration instead of `D` as before?
> > > Before this we were caching comment for every redeclaration separately but for every redeclaration the comment was the same.
> > > 
> > > As I understand it - for a given declaration we found the first comment in the redeclaration chain and then saved it to the cache for every redeclaration (the same comment).
> > > Later, during lookup, we iterated the redeclaration chain again and did a lookup for every redeclaration (see L392 in the original implementation).
> > > 
> > > Unless I missed something, it's suboptimal in both memory consumption and runtime overhead.
> > > 
> > > That's the reason why I want to cache the comment for the redeclaration group as a whole. The only thing I am not entirely sure about is what key to use to represent the whole redeclaration chain - maybe the first declaration in the redeclaration chain would be better then the canonical declaration...
> > > 
> > > WDYT?
> > > As I understand it - for a given declaration we found the first comment in the redeclaration chain and then saved it to the cache for every redeclaration (the same comment).
> > 
> > Only if there was only one comment in the redeclaration chain.  If a redeclaration had a different comment, this function would return that comment.
> I see. I made a wrong assumption - that for any two redeclarations the redeclaration chain always starts from the same declaration.
I still don't think this patch is a no-op.

Here is the intended behavior:

```
/// First
void foo(); // (1), canonical decl

void foo(); // (2)

/// Third
void foo() {} // (3)

void foo(); // (4)
```

getRawCommentForAnyRedecl(1) => "First"
getRawCommentForAnyRedecl(2) => "First"
getRawCommentForAnyRedecl(3) => "Third"
getRawCommentForAnyRedecl(4) => "First"



================
Comment at: clang/lib/AST/ASTContext.cpp:373
 
-  // Check whether we have cached a comment for this declaration already.
-  {
-    llvm::DenseMap<const Decl *, RawCommentAndCacheFlags>::iterator Pos =
-        RedeclComments.find(D);
+  auto CacheLookup = [&](const Decl *SpecificD) -> const RawComment * {
+    llvm::DenseMap<const Decl *, CommentAndOrigin>::iterator Pos =
----------------
`GetCommentFromCache`?


================
Comment at: clang/lib/AST/ASTContext.cpp:405
+  // Check whether we have cached a comment for this specific declaration.
+  if (auto * CachedComment = CacheLookup(D))
+    return CachedComment;
----------------
No space after `*`.


================
Comment at: clang/lib/AST/ASTContext.cpp:415
+
+  // In case this is the canonical Decl we're done.
+  auto CanonicalD = D->getCanonicalDecl();
----------------
I don't understand the comment -- I read it as "if we got here, and the decl was canonical, and we haven't found anything already, we should return nullptr".  The code does something different.

Also, just to be clear, here is the intended behavior:

```
void foo(); // (1), canonical decl

/// Second
void foo(); // (2)
```

getRawCommentForAnyRedecl(1) => "Second"
getRawCommentForAnyRedecl(2) => "Second"



================
Comment at: clang/lib/AST/ASTContext.cpp:418
+  if (!CanonicalD)
+    return nullptr;
 
----------------
When is it possible to not have a canonical decl?


================
Comment at: clang/lib/AST/ASTContext.cpp:421
+  // Check whether we have cached a comment for canonical declaration of this declaration.
+  if (auto * CachedComment = CacheLookup(CanonicalD))
+    return CachedComment;
----------------
No space after `*`.


================
Comment at: clang/lib/AST/ASTContext.cpp:425
+  // We don't have comment for neither D, nor it's canonical declaration in cache and D doesn't have any comment attached to itself.
+  // Search for any comment attached to redeclarations of D and cache it for canonical declaration of D.
   for (auto I : D->redecls()) {
----------------
80 columns.


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

https://reviews.llvm.org/D60432