[PATCH] D68467: [clangd] If an undocumented definition exists, don't accept documentation from other forward decls.

[Sam McCall via Phabricator via cfe-commits]

sammccall created this revision.
sammccall added a reviewer: ilya-biryukov.
Herald added subscribers: cfe-commits, usaxena95, kadircet, arphaman, jkorous, 
MaskRay.
Herald added a project: clang.

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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D68467

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


Index: clang-tools-extra/clangd/unittests/IndexTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IndexTests.cpp
+++ clang-tools-extra/clangd/unittests/IndexTests.cpp
@@ -413,6 +413,16 @@
                                        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);
 }
Index: clang-tools-extra/clangd/index/Merge.cpp
===================================================================
--- clang-tools-extra/clangd/index/Merge.cpp
+++ clang-tools-extra/clangd/index/Merge.cpp
@@ -186,7 +186,10 @@
     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;



Index: clang-tools-extra/clangd/unittests/IndexTests.cpp
===================================================================
--- clang-tools-extra/clangd/unittests/IndexTests.cpp
+++ clang-tools-extra/clangd/unittests/IndexTests.cpp
@@ -413,6 +413,16 @@
                                        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);
 }
Index: clang-tools-extra/clangd/index/Merge.cpp
===================================================================
--- clang-tools-extra/clangd/index/Merge.cpp
+++ clang-tools-extra/clangd/index/Merge.cpp
@@ -186,7 +186,10 @@
     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;

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits