[clang-tools-extra] r373892 - [clangd] If an undocumented definition exists, don't accept documentation from other forward decls.

[Sam McCall via cfe-commits]

Author: sammccall
Date: Mon Oct  7 03:53:56 2019
New Revision: 373892

URL: http://llvm.org/viewvc/llvm-project?rev=373892&view=rev
Log:
[clangd] If an undocumented definition exists, don't accept documentation from other forward decls.

Summary:
This fixes cases like:
  foo.h
    class Undocumented{}
  bar.h
    // break an include cycle. we should refactor this!
    class Undocumented;
Where the comment doesn't describe the class.

Note that a forward decl that is *visible to the definition* will still have
its doc comment used, by SymbolCollector: Merge isn't involved here.

Reviewers: ilya-biryukov

Subscribers: MaskRay, jkorous, arphaman, kadircet, usaxena95, cfe-commits

Tags: #clang

Differential Revision: https://reviews.llvm.org/D68467

Modified:
    clang-tools-extra/trunk/clangd/index/Merge.cpp
    clang-tools-extra/trunk/clangd/unittests/IndexTests.cpp

Modified: clang-tools-extra/trunk/clangd/index/Merge.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/index/Merge.cpp?rev=373892&r1=373891&r2=373892&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/index/Merge.cpp (original)
+++ clang-tools-extra/trunk/clangd/index/Merge.cpp Mon Oct  7 03:53:56 2019
@@ -186,7 +186,10 @@ Symbol mergeSymbol(const Symbol &L, cons
     S.Signature = O.Signature;
   if (S.CompletionSnippetSuffix == "")
     S.CompletionSnippetSuffix = O.CompletionSnippetSuffix;
-  if (S.Documentation == "")
+  // Don't accept documentation from bare forward declarations, if there is a
+  // definition and it didn't provide one. S is often an undocumented class,
+  // and O is a non-canonical forward decl preceded by an irrelevant comment.
+  if (S.Documentation == "" && !S.Definition)
     S.Documentation = O.Documentation;
   if (S.ReturnType == "")
     S.ReturnType = O.ReturnType;

Modified: clang-tools-extra/trunk/clangd/unittests/IndexTests.cpp
URL: http://llvm.org/viewvc/llvm-project/clang-tools-extra/trunk/clangd/unittests/IndexTests.cpp?rev=373892&r1=373891&r2=373892&view=diff
==============================================================================
--- clang-tools-extra/trunk/clangd/unittests/IndexTests.cpp (original)
+++ clang-tools-extra/trunk/clangd/unittests/IndexTests.cpp Mon Oct  7 03:53:56 2019
@@ -413,6 +413,16 @@ TEST(MergeIndexTest, Refs) {
                                        FileURI("unittest:///test2.cc"))))));
 }
 
+TEST(MergeIndexTest, NonDocumentation) {
+  Symbol L, R;
+  L.ID = R.ID = SymbolID("x");
+  L.Definition.FileURI = "file:/x.h";
+  R.Documentation = "Forward declarations because x.h is too big to include";
+
+  Symbol M = mergeSymbol(L, R);
+  EXPECT_EQ(M.Documentation, "");
+}
+
 MATCHER_P2(IncludeHeaderWithRef, IncludeHeader, References, "") {
   return (arg.IncludeHeader == IncludeHeader) && (arg.References == References);
 }