[clangd] Separate protobuf-related functions to a dedicated file.

[kadircet]

LG. I might be missing some context though, what's the reasoning behind? Because, I think it is not necessary to treat protobufs differently.

It might be sensible to have a more generic isSymbolFromGeneratedFile filter, but also for this one I don't see any developments in the near future.

clang-tools-extra/clangd/Protobuf.cpp

25

maybe move this comment down below, right before the line if (!SM.getBufferData(FID).startswith(PROTO_HEADER_COMMENT))

clang-tools-extra/clangd/index/SymbolCollector.cpp

81

looks like this fixme got dropped

[merge_guards_bot]

Unit tests: fail. 62288 tests passed, 8 failed and 837 were skipped.

failed: LLVM.CodeGen/AMDGPU/branch-relaxation.ll
failed: LLVM.CodeGen/AMDGPU/cf-loop-on-constant.ll
failed: LLVM.CodeGen/AMDGPU/infinite-loop.ll
failed: LLVM.CodeGen/AMDGPU/optimize-negated-cond.ll
failed: LLVM.CodeGen/AMDGPU/uniform-cfg.ll
failed: LLVM.CodeGen/AMDGPU/update-phi.ll
failed: LLVM.CodeGen/AMDGPU/vgpr-descriptor-waterfall-loop-idom-update.ll
failed: LLVM.Transforms/LoopStrengthReduce/AMDGPU/different-addrspace-crash.ll

clang-tidy: fail. clang-tidy found 0 errors and 3 warnings. 3 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[hokein]

update, and address comments.

[hokein]

In D73780#1851654, @kadircet wrote:

LG. I might be missing some context though, what's the reasoning behind? Because, I think it is not necessary to treat protobufs differently.

It might be sensible to have a more generic isSymbolFromGeneratedFile filter, but also for this one I don't see any developments in the near future.

the motivation of this change is that we will blacklist the proto symbols for rename, we need a place to put protobuf-related functions (and we might improve clangd on better supporting protobuf symbol navigation this year) , I think it is sensible to lift them into a separate file.

clang-tools-extra/clangd/index/SymbolCollector.cpp

81

oops..

[hokein]

add trailing blank line

[merge_guards_bot]

Unit tests: fail. 62288 tests passed, 8 failed and 837 were skipped.

failed: LLVM.CodeGen/AMDGPU/branch-relaxation.ll
failed: LLVM.CodeGen/AMDGPU/cf-loop-on-constant.ll
failed: LLVM.CodeGen/AMDGPU/infinite-loop.ll
failed: LLVM.CodeGen/AMDGPU/optimize-negated-cond.ll
failed: LLVM.CodeGen/AMDGPU/uniform-cfg.ll
failed: LLVM.CodeGen/AMDGPU/update-phi.ll
failed: LLVM.CodeGen/AMDGPU/vgpr-descriptor-waterfall-loop-idom-update.ll
failed: LLVM.Transforms/LoopStrengthReduce/AMDGPU/different-addrspace-crash.ll

clang-tidy: fail. clang-tidy found 0 errors and 2 warnings. 2 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[merge_guards_bot]

Unit tests: fail. 62288 tests passed, 8 failed and 837 were skipped.

failed: LLVM.CodeGen/AMDGPU/branch-relaxation.ll
failed: LLVM.CodeGen/AMDGPU/cf-loop-on-constant.ll
failed: LLVM.CodeGen/AMDGPU/infinite-loop.ll
failed: LLVM.CodeGen/AMDGPU/optimize-negated-cond.ll
failed: LLVM.CodeGen/AMDGPU/uniform-cfg.ll
failed: LLVM.CodeGen/AMDGPU/update-phi.ll
failed: LLVM.CodeGen/AMDGPU/vgpr-descriptor-waterfall-loop-idom-update.ll
failed: LLVM.Transforms/LoopStrengthReduce/AMDGPU/different-addrspace-crash.ll

clang-tidy: fail. clang-tidy found 0 errors and 2 warnings. 2 of them are added as review comments below (why?).

clang-format: pass.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[kadircet]

In D73780#1851920, @hokein wrote:

In D73780#1851654, @kadircet wrote:

LG. I might be missing some context though, what's the reasoning behind? Because, I think it is not necessary to treat protobufs differently.

It might be sensible to have a more generic isSymbolFromGeneratedFile filter, but also for this one I don't see any developments in the near future.

the motivation of this change is that we will blacklist the proto symbols for rename, we need a place to put protobuf-related functions (and we might improve clangd on better supporting protobuf symbol navigation this year) , I think it is sensible to lift them into a separate file.

if that's the case, I would still go with having a more generic isGeneratedSymbol, that only supports filtering protobuf-related symbols for now. and would rather put it into AST.h

[hokein]

update based on the offline discussion.

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

[kadircet]

LGTM, thanks!

clang-tools-extra/clangd/SourceCode.cpp

1139 ↗

(On Diff #242536)

nit: just return SM.getBufferData(FID).startswith(PROTO_HEADER_COMMENT);

[hokein]

address a final comment.

[merge_guards_bot]

Unit tests: unknown.

clang-tidy: unknown.

clang-format: unknown.

Build artifacts: console-log.txt

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

clang-tools-extra/clangd/CMakeLists.txt

add_clang_library(clangDaemon
   GlobalCompilationDatabase.cpp
   Headers.cpp
   HeaderSourceSwitch.cpp
   Hover.cpp
   IncludeFixer.cpp
   JSONTransport.cpp
   Logger.cpp
   PathMapping.cpp
+  Protobuf.cpp
   Protocol.cpp
   Quality.cpp
   ParsedAST.cpp
   Preamble.cpp
   RIFF.cpp
   Selection.cpp
   SemanticHighlighting.cpp
   SemanticSelection.cpp

clang-tools-extra/clangd/Protobuf.h

+//===--- Protobuf.h -  ------------------------------------------*- C++ -*-===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+#ifndef LLVM_CLANG_TOOLS_EXTRA_CLANGD_PROTOBUF_H
+#define LLVM_CLANG_TOOLS_EXTRA_CLANGD_PROTOBUF_H
+
+#include "clang/AST/Decl.h"
+
+namespace clang {
+namespace clangd {
+
+/// Checks whether the decl is a private symbol in a header generated by
+/// protobuf compiler.
+bool isPrivateProtoDecl(const NamedDecl &ND);
+
+} // namespace clangd
+} // namespace clang
+
+#endif // LLVM_CLANG_TOOLS_EXTRA_CLANGD_PROTOBUF_H

clang-tools-extra/clangd/Protobuf.cpp

+//===--- Protobuf.cpp -------------------------------------------*- C++ -*-===//
+//
+// 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 "Protobuf.h"
+#include "AST.h"
+#include "clang/AST/ASTContext.h"
+#include "clang/AST/Decl.h"
+
+namespace clang {
+namespace clangd {
+
+// All proto generated headers should start with this line.
+static const char *PROTO_HEADER_COMMENT =
+    "// Generated by the protocol buffer compiler.  DO NOT EDIT!";
+
+// FIXME: make filtering extensible when there are more use cases for symbol
+// filters.
+bool isPrivateProtoDecl(const NamedDecl &ND) {
+  const auto &SM = ND.getASTContext().getSourceManager();
+  auto Loc = nameLocation(ND, SM);
+  auto FileName = SM.getFilename(Loc);
+  if (!FileName.endswith(".proto.h") && !FileName.endswith(".pb.h"))
+    return false;
+  auto FID = SM.getFileID(Loc);
+  // To identify whether a proto header is actually generated by proto compiler,
+  // we check whether it starts with PROTO_HEADER_COMMENT.
+  if (!SM.getBufferData(FID).startswith(PROTO_HEADER_COMMENT))
+    return false;
+
+  // ND without identifier can be operators.
+  if (ND.getIdentifier() == nullptr)
+    return false;
+  auto Name = ND.getIdentifier()->getName();
+  if (!Name.contains('_'))
+    return false;
+  // Nested proto entities (e.g. Message::Nested) have top-level decls
+  // that shouldn't be used (Message_Nested). Ignore them completely.
+  // The nested entities are dangling type aliases, we may want to reconsider
+  // including them in the future.
+  // For enum constants, SOME_ENUM_CONSTANT is not private and should be
+  // indexed. Outer_INNER is private. This heuristic relies on naming style, it
+  // will include OUTER_INNER and exclude some_enum_constant.
+  // FIXME: the heuristic relies on naming style (i.e. no underscore in
+  // user-defined names) and can be improved.
+  return (ND.getKind() != Decl::EnumConstant) || llvm::any_of(Name, islower);
+}
+
+} // namespace clangd
+} // namespace clang
 No newline at end of file

clang-tools-extra/clangd/index/SymbolCollector.cpp

 //===--- SymbolCollector.cpp -------------------------------------*- C++-*-===//
 //
 // 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 "SymbolCollector.h"
 #include "AST.h"
 #include "CanonicalIncludes.h"
 #include "CodeComplete.h"
 #include "CodeCompletionStrings.h"
 #include "ExpectedTypes.h"
 #include "Logger.h"
+#include "Protobuf.h"
 #include "SourceCode.h"
 #include "SymbolLocation.h"
 #include "URI.h"
 #include "index/SymbolID.h"
 #include "clang/AST/Decl.h"
 #include "clang/AST/DeclBase.h"
 #include "clang/AST/DeclCXX.h"
 #include "clang/AST/DeclTemplate.h"
std::string toURI(const SourceManager &SM, llvm::StringRef Path,
   // We don't perform is_absolute check in an else branch because makeAbsolute
   // might return a relative path on some InMemoryFileSystems.
   if (!llvm::sys::path::is_absolute(AbsolutePath) && !Opts.FallbackDir.empty())
     llvm::sys::fs::make_absolute(Opts.FallbackDir, AbsolutePath);
   llvm::sys::path::remove_dots(AbsolutePath, /*remove_dot_dot=*/true);
   return URI::create(AbsolutePath).toString();
 }
 
-// All proto generated headers should start with this line.
-static const char *PROTO_HEADER_COMMENT =
-    "// Generated by the protocol buffer compiler.  DO NOT EDIT!";
-
-// Checks whether the decl is a private symbol in a header generated by
-// protobuf compiler.
-// To identify whether a proto header is actually generated by proto compiler,
-// we check whether it starts with PROTO_HEADER_COMMENT.
-// FIXME: make filtering extensible when there are more use cases for symbol
-// filters.
-bool isPrivateProtoDecl(const NamedDecl &ND) {
-  const auto &SM = ND.getASTContext().getSourceManager();
-  auto Loc = nameLocation(ND, SM);
-  auto FileName = SM.getFilename(Loc);
-  if (!FileName.endswith(".proto.h") && !FileName.endswith(".pb.h"))
-    return false;
-  auto FID = SM.getFileID(Loc);
-  // Double check that this is an actual protobuf header.
-  if (!SM.getBufferData(FID).startswith(PROTO_HEADER_COMMENT))
-    return false;
-
-  // ND without identifier can be operators.
-  if (ND.getIdentifier() == nullptr)
-    return false;
-  auto Name = ND.getIdentifier()->getName();
-  if (!Name.contains('_'))
-    return false;
-  // Nested proto entities (e.g. Message::Nested) have top-level decls
-  // that shouldn't be used (Message_Nested). Ignore them completely.
-  // The nested entities are dangling type aliases, we may want to reconsider
-  // including them in the future.
-  // For enum constants, SOME_ENUM_CONSTANT is not private and should be
-  // indexed. Outer_INNER is private. This heuristic relies on naming style, it
-  // will include OUTER_INNER and exclude some_enum_constant.
-  // FIXME: the heuristic relies on naming style (i.e. no underscore in
-  // user-defined names) and can be improved.
-  return (ND.getKind() != Decl::EnumConstant) || llvm::any_of(Name, islower);
-}
-
 // We only collect #include paths for symbols that are suitable for global code
 // completion, except for namespaces since #include path for a namespace is hard
 // to define.
 bool shouldCollectIncludePath(index::SymbolKind Kind) {
   using SK = index::SymbolKind;
   switch (Kind) {
   case SK::Macro:
   case SK::Enum: