[clang] c336c98 - [C++20] [Modules] [Serialization] Don't write comments to BMI for C++20 Named Modules

[Chuanqi Xu via cfe-commits]

Author: Chuanqi Xu
Date: 2023-06-06T13:05:17+08:00
New Revision: c336c983bcd9bf3559c76ed4eb6b25fbafbcd361

URL: https://github.com/llvm/llvm-project/commit/c336c983bcd9bf3559c76ed4eb6b25fbafbcd361
DIFF: https://github.com/llvm/llvm-project/commit/c336c983bcd9bf3559c76ed4eb6b25fbafbcd361.diff

LOG: [C++20] [Modules] [Serialization] Don't write comments to BMI for C++20 Named Modules

This patch forbids to write comment to BMIs for C++20 Named Modules.
Originally I thought this was helpful for language services like clangd.
But I found clangd don't want the BMI to contain comments actually. So
it is meaningless for C++20 Named Modules to keep such comments in
their BMI.

It is simple to enable this when someday we found we want this actually.

Added: 
    clang/unittests/Serialization/NoCommentsTest.cpp

Modified: 
    clang/lib/Serialization/ASTWriter.cpp
    clang/unittests/Serialization/CMakeLists.txt

Removed: 
    


################################################################################
diff  --git a/clang/lib/Serialization/ASTWriter.cpp b/clang/lib/Serialization/ASTWriter.cpp
index ee636028b8e25..5ac3030a7a676 100644
--- a/clang/lib/Serialization/ASTWriter.cpp
+++ b/clang/lib/Serialization/ASTWriter.cpp
@@ -3178,6 +3178,13 @@ void ASTWriter::WriteComments() {
   auto _ = llvm::make_scope_exit([this] { Stream.ExitBlock(); });
   if (!PP->getPreprocessorOpts().WriteCommentListToPCH)
     return;
+
+  // Don't write comments to BMI to reduce the size of BMI.
+  // If language services (e.g., clangd) want such abilities,
+  // we can offer a special option then.
+  if (isWritingStdCXXNamedModules())
+    return;
+
   RecordData Record;
   for (const auto &FO : Context->Comments.OrderedComments) {
     for (const auto &OC : FO.second) {

diff  --git a/clang/unittests/Serialization/CMakeLists.txt b/clang/unittests/Serialization/CMakeLists.txt
index 44e4ecb314365..d4dfb54af3d70 100644
--- a/clang/unittests/Serialization/CMakeLists.txt
+++ b/clang/unittests/Serialization/CMakeLists.txt
@@ -8,6 +8,7 @@ set(LLVM_LINK_COMPONENTS
 add_clang_unittest(SerializationTests
   InMemoryModuleCacheTest.cpp
   ModuleCacheTest.cpp
+  NoCommentsTest.cpp
   SourceLocationEncodingTest.cpp
   VarDeclConstantInitTest.cpp
   )

diff  --git a/clang/unittests/Serialization/NoCommentsTest.cpp b/clang/unittests/Serialization/NoCommentsTest.cpp
new file mode 100644
index 0000000000000..48f76bbd9d5f8
--- /dev/null
+++ b/clang/unittests/Serialization/NoCommentsTest.cpp
@@ -0,0 +1,126 @@
+//===- unittests/Serialization/NoComments.cpp - CI tests -----===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+#include "clang/ASTMatchers/ASTMatchFinder.h"
+#include "clang/ASTMatchers/ASTMatchers.h"
+#include "clang/Basic/FileManager.h"
+#include "clang/Frontend/CompilerInstance.h"
+#include "clang/Frontend/CompilerInvocation.h"
+#include "clang/Frontend/FrontendActions.h"
+#include "clang/Frontend/Utils.h"
+#include "clang/Lex/HeaderSearch.h"
+#include "clang/Tooling/Tooling.h"
+#include "llvm/ADT/SmallString.h"
+#include "llvm/Support/FileSystem.h"
+#include "llvm/Support/raw_ostream.h"
+
+#include "gtest/gtest.h"
+
+using namespace llvm;
+using namespace clang;
+
+namespace {
+
+class NoComments : public ::testing::Test {
+  void SetUp() override {
+    ASSERT_FALSE(
+        sys::fs::createUniqueDirectory("modules-no-comments-test", TestDir));
+  }
+
+  void TearDown() override { sys::fs::remove_directories(TestDir); }
+
+public:
+  SmallString<256> TestDir;
+
+  void addFile(StringRef Path, StringRef Contents) {
+    ASSERT_FALSE(sys::path::is_absolute(Path));
+
+    SmallString<256> AbsPath(TestDir);
+    sys::path::append(AbsPath, Path);
+
+    ASSERT_FALSE(
+        sys::fs::create_directories(llvm::sys::path::parent_path(AbsPath)));
+
+    std::error_code EC;
+    llvm::raw_fd_ostream OS(AbsPath, EC);
+    ASSERT_FALSE(EC);
+    OS << Contents;
+  }
+};
+
+TEST_F(NoComments, NonModulesTest) {
+  std::unique_ptr<ASTUnit> AST = tooling::buildASTFromCodeWithArgs(
+      R"cpp(
+/// Any comments
+void foo() {}
+        )cpp",
+      /*Args=*/{"-std=c++20"});
+  EXPECT_TRUE(AST);
+
+  ASTContext &Ctx = AST->getASTContext();
+
+  using namespace clang::ast_matchers;
+  auto *foo = selectFirst<FunctionDecl>(
+      "foo", match(functionDecl(hasName("foo")).bind("foo"), Ctx));
+  EXPECT_TRUE(foo);
+
+  const RawComment *RC = getCompletionComment(Ctx, foo);
+  EXPECT_TRUE(RC);
+  EXPECT_TRUE(RC->getRawText(Ctx.getSourceManager()).trim() ==
+              "/// Any comments");
+}
+
+TEST_F(NoComments, ModulesTest) {
+  addFile("Comments.cppm", R"cpp(
+export module Comments;
+
+/// Any comments
+void foo() {}
+  )cpp");
+
+  IntrusiveRefCntPtr<DiagnosticsEngine> Diags =
+      CompilerInstance::createDiagnostics(new DiagnosticOptions());
+  CreateInvocationOptions CIOpts;
+  CIOpts.Diags = Diags;
+
+  std::string CacheBMIPath = llvm::Twine(TestDir + "/Comments.pcm").str();
+  const char *Args[] = {
+      "clang++",       "-std=c++20",    "--precompile", "-working-directory",
+      TestDir.c_str(), "Comments.cppm", "-o",           CacheBMIPath.c_str()};
+  std::shared_ptr<CompilerInvocation> Invocation =
+      createInvocation(Args, CIOpts);
+  ASSERT_TRUE(Invocation);
+
+  CompilerInstance Instance;
+  Instance.setDiagnostics(Diags.get());
+  Instance.setInvocation(Invocation);
+  GenerateModuleInterfaceAction Action;
+  ASSERT_TRUE(Instance.ExecuteAction(Action));
+  ASSERT_FALSE(Diags->hasErrorOccurred());
+
+  std::string DepArg =
+      llvm::Twine("-fmodule-file=Comments=" + CacheBMIPath).str();
+  std::unique_ptr<ASTUnit> AST = tooling::buildASTFromCodeWithArgs(
+      R"cpp(
+import Comments;
+        )cpp",
+      /*Args=*/{"-std=c++20", DepArg.c_str()});
+  EXPECT_TRUE(AST);
+
+  ASTContext &Ctx = AST->getASTContext();
+
+  using namespace clang::ast_matchers;
+  auto *foo = selectFirst<FunctionDecl>(
+      "foo", match(functionDecl(hasName("foo")).bind("foo"), Ctx));
+  EXPECT_TRUE(foo);
+
+  const RawComment *RC = getCompletionComment(Ctx, foo);
+  EXPECT_FALSE(RC);
+}
+
+} // anonymous namespace