This is an archive of the discontinued LLVM Phabricator instance.

Differential D73780

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

Authored by hokein on Jan 31 2020, 5:47 AM.

Details

Reviewers
kadircet
Commits
rGf8865c01944f: [clangd] Pull out a isProtoFile function.

Diff Detail

Event Timeline

hokein created this revision.Jan 31 2020, 5:47 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 31 2020, 5:47 AM
Herald added subscribers: usaxena95, arphaman, jkorous and 3 others. · View Herald Transcript
kadircet added a comment.Jan 31 2020, 6:00 AM

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
24 ↗(On Diff #241705)

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 added a subscriber: merge_guards_bot.Jan 31 2020, 6:24 AM

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.

Harbormaster failed remote builds in B45442: Diff 241705!Jan 31 2020, 6:25 AM
hokein updated this revision to Diff 241734.Jan 31 2020, 7:59 AM
hokein marked 3 inline comments as done.

update, and address comments.

hokein added a comment.Jan 31 2020, 8:00 AM
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 updated this revision to Diff 241735.Jan 31 2020, 8:00 AM

add trailing blank line

merge_guards_bot added a comment.Jan 31 2020, 8:10 AM

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.

Harbormaster failed remote builds in B45458: Diff 241734!Jan 31 2020, 8:10 AM
merge_guards_bot added a comment.Jan 31 2020, 8:20 AM

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.

Harbormaster failed remote builds in B45459: Diff 241735!Jan 31 2020, 8:20 AM
kadircet added a comment.Feb 4 2020, 6:37 AM
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 updated this revision to Diff 242536.Feb 5 2020, 2:06 AM

update based on the offline discussion.

merge_guards_bot added a comment.Feb 5 2020, 2:12 AM

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.

Harbormaster failed remote builds in B45751: Diff 242536!Feb 5 2020, 2:12 AM
kadircet accepted this revision.Feb 5 2020, 2:37 AM

LGTM, thanks!

clang-tools-extra/clangd/SourceCode.cpp
1139

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

This revision is now accepted and ready to land.Feb 5 2020, 2:37 AM
hokein updated this revision to Diff 242543.Feb 5 2020, 2:54 AM

address a final comment.

merge_guards_bot added a comment.Feb 5 2020, 2:57 AM

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.

Harbormaster failed remote builds in B45753: Diff 242543!Feb 5 2020, 2:57 AM
Closed by commit rGf8865c01944f: [clangd] Pull out a isProtoFile function. (authored by hokein). · Explain WhyFeb 5 2020, 3:15 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
clang-tools-extra/
clangd/
3 lines
12 lines
index/
14 lines

Diff 242546

clang-tools-extra/clangd/SourceCode.h

Show First 20 Lines • Show All 292 Lines • ▼ Show 20 Lines
llvm::Optional<DefinedMacro> locateMacroAt(SourceLocation Loc, llvm::Optional<DefinedMacro> locateMacroAt(SourceLocation Loc,
Preprocessor &PP); Preprocessor &PP);
/// Infers whether this is a header from the FileName and LangOpts (if /// Infers whether this is a header from the FileName and LangOpts (if
/// presents). /// presents).
bool isHeaderFile(llvm::StringRef FileName, bool isHeaderFile(llvm::StringRef FileName,
llvm::Optional<LangOptions> LangOpts = llvm::None); llvm::Optional<LangOptions> LangOpts = llvm::None);
/// Returns true if the given location is in a generated protobuf file.
bool isProtoFile(SourceLocation Loc, const SourceManager &SourceMgr);
} // namespace clangd } // namespace clangd
} // namespace clang } // namespace clang
#endif #endif

clang-tools-extra/clangd/SourceCode.cpp

Show First 20 Lines • Show All 1,121 Lines • ▼ Show 20 Linesbool isHeaderFile(llvm::StringRef FileName,
if (LangOpts && LangOpts->IsHeaderFile) if (LangOpts && LangOpts->IsHeaderFile)
return true; return true;
namespace types = clang::driver::types; namespace types = clang::driver::types;
auto Lang = types::lookupTypeForExtension( auto Lang = types::lookupTypeForExtension(
llvm::sys::path::extension(FileName).substr(1)); llvm::sys::path::extension(FileName).substr(1));
return Lang != types::TY_INVALID && types::onlyPrecompileType(Lang); return Lang != types::TY_INVALID && types::onlyPrecompileType(Lang);
} }
bool isProtoFile(SourceLocation Loc, const SourceManager &SM) {
auto FileName = SM.getFilename(Loc);
if (!FileName.endswith(".proto.h") && !FileName.endswith(".pb.h"))
return false;
auto FID = SM.getFileID(Loc);
// All proto generated headers should start with this line.
static const char *PROTO_HEADER_COMMENT =
"// Generated by the protocol buffer compiler. DO NOT EDIT!";
// Double check that this is an actual protobuf header.
return SM.getBufferData(FID).startswith(PROTO_HEADER_COMMENT);
kadircetUnsubmitted
Not Done
ReplyInline Actions

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

kadircet: nit: just `return SM.getBufferData(FID).startswith(PROTO_HEADER_COMMENT);`
}
} // namespace clangd } // namespace clangd
} // namespace clang } // namespace clang

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

Show First 20 Lines • Show All 64 Lines • ▼ Show 20 Linesstd::string toURI(const SourceManager &SM, llvm::StringRef Path,
// We don't perform is_absolute check in an else branch because makeAbsolute // We don't perform is_absolute check in an else branch because makeAbsolute
// might return a relative path on some InMemoryFileSystems. // might return a relative path on some InMemoryFileSystems.
if (!llvm::sys::path::is_absolute(AbsolutePath) && !Opts.FallbackDir.empty()) if (!llvm::sys::path::is_absolute(AbsolutePath) && !Opts.FallbackDir.empty())
llvm::sys::fs::make_absolute(Opts.FallbackDir, AbsolutePath); llvm::sys::fs::make_absolute(Opts.FallbackDir, AbsolutePath);
llvm::sys::path::remove_dots(AbsolutePath, /*remove_dot_dot=*/true); llvm::sys::path::remove_dots(AbsolutePath, /*remove_dot_dot=*/true);
return URI::create(AbsolutePath).toString(); 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 // Checks whether the decl is a private symbol in a header generated by
// protobuf compiler. // 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 // FIXME: make filtering extensible when there are more use cases for symbol
kadircetUnsubmitted
Done
ReplyInline Actions

looks like this fixme got dropped

kadircet: looks like this fixme got dropped
hokeinAuthorUnsubmitted
Done
ReplyInline Actions

oops..

hokein: oops..
// filters. // filters.
bool isPrivateProtoDecl(const NamedDecl &ND) { bool isPrivateProtoDecl(const NamedDecl &ND) {
const auto &SM = ND.getASTContext().getSourceManager(); const auto &SM = ND.getASTContext().getSourceManager();
auto Loc = nameLocation(ND, SM); if (!isProtoFile(nameLocation(ND, SM), 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; return false;
// ND without identifier can be operators. // ND without identifier can be operators.
if (ND.getIdentifier() == nullptr) if (ND.getIdentifier() == nullptr)
return false; return false;
auto Name = ND.getIdentifier()->getName(); auto Name = ND.getIdentifier()->getName();
if (!Name.contains('_')) if (!Name.contains('_'))
return false; return false;
▲ Show 20 Lines • Show All 686 Lines • Show Last 20 Lines