[PATCH] D50375: [clangd] Share getSymbolID implementation.

hokein created this revision. hokein added a reviewer: ioeric. Herald added subscribers: arphaman, jkorous, MaskRay, ilya-biryukov. And remove all duplicated implementation. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50375 Files: clangd/AST.cpp clangd/AST.h clangd/Cod

[PATCH] D50375: [clangd] Share getSymbolID implementation.

hokein added inline comments. Comment at: clangd/CodeComplete.cpp:399 case CodeCompletionResult::RK_Pattern: { -llvm::SmallString<128> USR; -if (/*Ignore=*/clang::index::generateUSRForDecl(R.Declaration, USR)) - return None; -return SymbolID(USR); +return

[PATCH] D50375: [clangd] Share getSymbolID implementation.

hokein updated this revision to Diff 159466. hokein marked 3 inline comments as done. hokein added a comment. Address comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50375 Files: clangd/AST.cpp clangd/AST.h clangd/CodeComplete.cpp clangd/XRefs.cpp clangd/inde

[PATCH] D50375: [clangd] Share getSymbolID implementation.

This revision was automatically updated to reflect the committed changes. Closed by commit rL339116: [clangd] Share getSymbolID implementation. (authored by hokein, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D50375 Files: clang-tool

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

hokein created this revision. hokein added reviewers: ilya-biryukov, ioeric. Herald added subscribers: arphaman, mgrang, jkorous, MaskRay, mgorny. This patch implements a SymbolOccurenceCollector, which will be used to: - Find all occurrences in AST - Find all occurrences in MemIndex Repository

[PATCH] D50438: [clangd] Sort GoToDefinition results.

hokein created this revision. hokein added a reviewer: ilya-biryukov. Herald added subscribers: arphaman, mgrang, jkorous, MaskRay, ioeric. GoToDefinition returns all declaration results (implicit/explicit) that are in the same location, and the results are returned in arbitrary order. Some LSP c

[PATCH] D50389: [clang-tidy] new check for Abseil

hokein added a comment. Thanks for contributing to clang-tidy! Comment at: docs/clang-tidy/checks/abseil-duration-division.rst:3 + +abseil-duration-division + This is a nice document. Does abseil have similar thing in its official gui

[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.

hokein created this revision. hokein added reviewers: ilya-biryukov, alexfh. Herald added a subscriber: xazax.hun. The upstream change r336737 causes a regression issue in our internal base codebase. Adding an option to the check allowing us whitelist these classes. Repository: rCTE Clang Too

[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.

hokein added a comment. In https://reviews.llvm.org/D50447#1192280, @JonasToth wrote: > If whitelisting works, thats fine. But I agree with @lebedev.ri that a > contribution/improvement to the ExprMutAnalyzer is the better option. This is > especially the case, because multiple checks (and mayb

[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.

hokein added a comment. > That looks remarkably like google benchmark main loop. (i don't know at hand > if making this const will break it, but probably not?) > I wonder if instead there should be an option to not complain about the > variables that aren't actually used? Yeah, it is google be

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

hokein added a comment. Sorry for the delay. I thought there was a dependent patch of this patch, but it seems resolved, right? A rough round of review. > New version. I tried to share the code between this and SymbolCollector, but > didn't find a good clean way. Do you have concrete suggestio

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

hokein added a comment. In https://reviews.llvm.org/D50385#1191914, @ioeric wrote: > 2 high-level questions: > > 1. What's the reason for having a separate `SymbolOccurrenceSlab`? Could > store occurrences as extra payload of `Symbol`? Storing occurrences in `Symbol` structure is easy to misus

[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.

hokein updated this revision to Diff 159919. hokein marked an inline comment as done. hokein added a comment. Adress review comment - ignore the case in the check. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50447 Files: clang-tidy/performance/ForRangeCopyCheck.cpp test/

[PATCH] D50447: [clang-tidy] Add a whitelistClasses option in performance-for-range-copy check.

hokein added inline comments. Comment at: clang-tidy/performance/ForRangeCopyCheck.cpp:39 + auto WhitelistClassMatcher = + cxxRecordDecl(hasAnyName(SmallVector( + WhitelistClasses.begin(), WhitelistClasses.end(; JonasToth wrote: > I have seens

[PATCH] D50447: [clang-tidy] Omit cases where loop variable is not used in loop body in performance-for-range-copy.

This revision was automatically updated to reflect the committed changes. Closed by commit rL339415: [clang-tidy] Omit cases where loop variable is not used in loop body in (authored by hokein, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.or

[PATCH] D48687: [clangd] Avoid duplicates in findDefinitions response

hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. Looks good with a few nits. Looks like you didn't update the patch correctly. You have marked comments done, but your code doesn't get changed accordingly. Please double check with it. I trie

[PATCH] D50389: [clang-tidy] Abseil: integral division of Duration check

hokein added inline comments. Comment at: clang-tidy/abseil/DurationDivisionCheck.cpp:24 + + const auto IsDuration = + expr(hasType(cxxRecordDecl(hasName("::absl::Duration"; maybe call it `DurationExpr` since you have declared the variable as `expr(...

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

hokein added a comment. The check is missing its document, please add one in `docs/clang-tidy/checks/`. Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23 + + Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"), + this);

[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

hokein added inline comments. Comment at: docs/clang-tidy/checks/abseil-no-internal-deps.rst:6 + +Gives a warning if code using Abseil depends on internal details. If something is in a namespace or filename/path that includes the word “internal”, code is not allowed to depend u

[PATCH] D50627: [clangd] Add a testcase for empty preamble.

hokein created this revision. hokein added a reviewer: ilya-biryukov. Herald added subscribers: arphaman, jkorous, MaskRay, ioeric, javed.absar. clangd maintains the last good preamble for each TU and clang treats an empty preamble as an error, therefore, clangd will use the stale preamble for a

[PATCH] D50628: [Preamble] Empty preamble is not an error.

hokein created this revision. hokein added a reviewer: ilya-biryukov. Empty preamble is valid for source file which doesn't have any preprocessor and #includes. This patch makes clang treat an empty preamble as a normal preamble. Check: ninja check-clang A testcase is added in https://reviews.l

[PATCH] D49885: Thread safety analysis: Allow relockable scopes

hokein added a comment. Hello, this patch seems introduce a new crash, and I have reverted it in r339558. Here is the minimal test case: class SCOPED_LOCKABLE FileLock { public: explicit FileLock() EXCLUSIVE_LOCK_FUNCTION(file_); ~FileLock() UNLOCK_FUNCTION(file_); //vo

[PATCH] D50635: Fix lint tests for D50449

hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. LGTM. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50635 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.

[PATCH] D50645: [clangd] Show non-instantiated decls in signatureHelp

hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. looks good. Comment at: clangd/CodeComplete.cpp:721 + // would get 'std::basic_string'). + if (auto Func = Candidate.getFunction()) { +if (auto Pattern = Fu

[PATCH] D50627: [clangd] Add a testcase for empty preamble.

hokein added a comment. In https://reviews.llvm.org/D50627#1196988, @ilya-biryukov wrote: > Maybe also add a test for find-definition that was broken before? (non-empty > preamble -> empty preamble -> bad gotodef that goes to included file instead > of the local variable) > To have a regressio

[PATCH] D50695: [clangd] Always use the latest preamble

hokein created this revision. hokein added a reviewer: ilya-biryukov. Herald added subscribers: arphaman, jkorous, MaskRay, ioeric, javed.absar. Fix an inconsistent behavior of using `LastBuiltPreamble`/`NewPreamble` in TUScheduler (see the test for details), AST should always use NewPreamble. Thi

[PATCH] D50438: [clangd] Sort GoToDefinition results.

hokein updated this revision to Diff 160544. hokein marked 3 inline comments as done. hokein added a comment. Address review comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50438 Files: clangd/XRefs.cpp unittests/clangd/XRefsTests.cpp Index: unittests/clangd/XRefs

[PATCH] D50389: [clang-tidy] Abseil: integral division of Duration check

hokein added a comment. Looks mostly good. Comment at: test/clang-tidy/abseil-duration-division.cpp:58 + // CHECK-MESSAGES: [[@LINE-4]]:45: warning: operator/ on absl::Duration objects + // CHECK-FIXES: double DoubleDivision(T t1, T t2) {return + // absl::FDivDuration(t1, t

[PATCH] D50389: [clang-tidy] Abseil: integral division of Duration check

hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. Thanks, looks good from my side. I will commit for you if other reviewers have no further comments. https://reviews.llvm.org/D50389 ___ cfe-comm

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

hokein accepted this revision. hokein added a comment. looks good, a few nits. Comment at: clangd/CodeComplete.cpp:742 +llvm::DenseMap FetchedDocs; +if (Index) { + LookupRequest IndexRequest; nit: do we want to log anything here? It may be useful f

[PATCH] D50695: [clangd] Always use the latest preamble

hokein updated this revision to Diff 160983. hokein marked 4 inline comments as done. hokein added a comment. Address review comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50695 Files: clangd/TUScheduler.cpp unittests/clangd/XRefsTests.cpp Index: unittests/clang

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

hokein added inline comments. Comment at: clangd/index/Index.h:65 public: + static llvm::Optional forDecl(const Decl &D); + ilya-biryukov wrote: > hokein wrote: > > We already have this similar function in clangd/AST.h. > Thanks for pointing this out. > It's so

[PATCH] D50627: [clangd] Add a testcase for empty preamble.

hokein updated this revision to Diff 161000. hokein marked 2 inline comments as done. hokein added a comment. Address review comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50627 Files: unittests/clangd/TUSchedulerTests.cpp Index: unittests/clangd/TUSchedulerTests.

[PATCH] D50727: [clangd] Fetch documentation from the Index during signature help

hokein added inline comments. Comment at: clangd/CodeComplete.cpp:755 + }); + log("SigHelp: requested docs for {0} symbols from the index, got {1} " + "symbols with non-empty docs in the response", ioeric wrote: > ilya-biryukov wrote: > > ioeri

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

hokein added inline comments. Comment at: clang-tidy/abseil/NoNamespaceCheck.cpp:23 + + Finder->addMatcher(namespaceDecl(hasName("absl")).bind("absl_namespace"), + this); JonasToth wrote: > hugoeg wrote: > > deannagarcia wrote: > > > aaron.ba

[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

hokein added a comment. @hugoeg, it looks like you are waiting for review, but this patch doesn't include the `IsExpansionInAbseilHeader` matcher. What's your plan of it? Comment at: test/clang-tidy/abseil-fake-declarations.h:1 +namespace std { +struct string { ---

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

hokein added a comment. > Hmm, I think this is the same for other symbol payload e.g. definition can be > missing for a symbol. And it seems to me that the concern is on the > SymbolSlab level: if a slab is for a single TU, users should expect missing > information; if a slab is merged from all

[PATCH] D48714: [clang-tidy] fix PR37913, templated exception factory diagnosed correctly

hokein added inline comments. Comment at: test/clang-tidy/hicpp-exception-baseclass.cpp:191 +void templated_thrower() { throw T{}(); } +// CHECK-MESSAGES: [[@LINE-1]]:34: warning: throwing an exception whose type 'int' is not derived from 'std::exception' + Jona

[PATCH] D50627: [clangd] Add a testcase for empty preamble.

hokein updated this revision to Diff 161176. hokein marked an inline comment as done. hokein added a comment. Remove ASSERT_TRUE(bool(Preamble)). Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50627 Files: unittests/clangd/TUSchedulerTests.cpp Index: unittests/clangd/TUSche

[PATCH] D50695: [clangd] Always use the latest preamble

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE340001: [clangd] Always use the latest preamble (authored by hokein, committed by ). Changed prior to commit: https://reviews.llvm.org/D50695?vs=160983&id=161177#toc Repository: rCTE Clang Tools Ex

[PATCH] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

hokein added a comment. Thanks for adding it. Comment at: clangd/TUScheduler.cpp:406 // We only need to build the AST if diagnostics were requested. if (WantDiags == WantDiagnostics::No) return; The AST might not get built if `WantDiags::No`, a

[PATCH] D50896: [clangd] Add xrefs LSP boilerplate implementation.

hokein created this revision. hokein added reviewers: ilya-biryukov, ioeric. Herald added subscribers: arphaman, jkorous, MaskRay. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50896 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/ClangdServer.cpp clang

[PATCH] D50862: [clang-tidy] Abseil: faster strsplit delimiter check

hokein added a subscriber: sbenza. hokein added a comment. Thanks for porting the check to upstream (context: the check was developed internally, and has been run on our codebase for a while, it is pretty stable). Could you please update the patch message to indicate this is a porting change, a

[PATCH] D50628: [Preamble] Empty preamble is not an error.

This revision was automatically updated to reflect the committed changes. Closed by commit rC340029: [Preamble] Empty preamble is not an error. (authored by hokein, committed by ). Changed prior to commit: https://reviews.llvm.org/D50628?vs=160319&id=161241#toc Repository: rC Clang https://

[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

hokein added inline comments. Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:20 + +bool IsInAbseilFile(const SourceManager& manager, SourceLocation loc){ + if (loc.isInvalid()) { I think we can make it as an ASTMatcher instead of a function like: ``` AST

[PATCH] D50896: [clangd] Add xrefs LSP boilerplate implementation.

hokein added inline comments. Comment at: clangd/XRefs.cpp:665 +std::vector references(ParsedAST &AST, Position Pos, + bool IncludeDeclaration, + const SymbolIndex *Index) { ilya-biryukov wrote: > Ar

[PATCH] D50627: [clangd] Add a testcase for empty preamble.

This revision was automatically updated to reflect the committed changes. Closed by commit rL340035: [clangd] Add a testcase for empty preamble. (authored by hokein, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.llvm.org/D50627 Files: clang-too

[PATCH] D50389: [clang-tidy] Abseil: integral division of Duration check

hokein closed this revision. hokein added a comment. Committed in https://reviews.llvm.org/rL340038. https://reviews.llvm.org/D50389 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D50896: [clangd] Add xrefs LSP boilerplate implementation.

hokein added a comment. In https://reviews.llvm.org/D50896#1204310, @ilya-biryukov wrote: > This change LG, but I would not commit it before we have an actual > implementation. > As soon as we have the `references` function in `ClangdUnit.cpp` > implemented, the merge of this change should be

[PATCH] D50389: [clang-tidy] Abseil: integral division of Duration check

hokein added a comment. In https://reviews.llvm.org/D50389#1204514, @Eugene.Zelenko wrote: > Somehow documentation file was not committed. Oops, I forgot to `git add` to the doc file. `arc patch` somehow failed to apply this patch, I applied it manually. Added in https://reviews.llvm.org/rL34

[PATCH] D50958: [clangd] WIP: xrefs for dynamic index.

hokein created this revision. hokein added reviewers: ilya-biryukov, ioeric. Herald added subscribers: kadircet, arphaman, mgrang, jkorous, MaskRay, javed.absar, mgorny. Depends on the AST callback interfaces. - https://reviews.llvm.org/D50847 - https://reviews.llvm.org/D50889 Repository: rC

[PATCH] D50958: [clangd] WIP: xrefs for dynamic index.

hokein updated this revision to Diff 161441. hokein added a comment. More cleanups. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50958 Files: clangd/ClangdLSPServer.cpp clangd/ClangdLSPServer.h clangd/ClangdServer.cpp clangd/ClangdServer.h clangd/Protocol.cpp clan

[PATCH] D50958: [clangd] WIP: xrefs for dynamic index.

hokein added a comment. This patch is incomplete, and it is a **prototype** based on our discussion last week. You can patch it locally, and play around with VSCode. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50958 ___ cfe-comm

[PATCH] D50960: [clangd] Add missing lock in the lookup.

hokein created this revision. hokein added a reviewer: ioeric. Herald added subscribers: kadircet, arphaman, jkorous, MaskRay, ilya-biryukov. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50960 Files: clangd/index/MemIndex.cpp Index: clangd/index/MemIndex.cpp ===

[PATCH] D50961: [clang-tidy] Add missing check documentation.

hokein created this revision. hokein added a reviewer: ioeric. Herald added subscribers: kadircet, arphaman, jkorous, ilya-biryukov, xazax.hun. [clangd] Simplify the code using UniqueStringSaver, NFC. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50961 Files: clangd/index/In

[PATCH] D50961: [clangd] Simplify the code using UniqueStringSaver, NFC.

hokein updated this revision to Diff 161445. hokein added a comment. Correct the patch. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50961 Files: clangd/index/Index.cpp clangd/index/Index.h docs/clang-tidy/checks/abseil-duration-division.rst Index: docs/clang-tidy/chec

[PATCH] D50961: [clangd] Simplify the code using UniqueStringSaver, NFC.

hokein updated this revision to Diff 161446. hokein added a comment. Update Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50961 Files: clangd/index/Index.cpp clangd/index/Index.h Index: clangd/index/Index.h

[PATCH] D50960: [clangd] Add missing lock in the lookup.

This revision was automatically updated to reflect the committed changes. Closed by commit rCTE340156: [clangd] Add missing lock in the lookup. (authored by hokein, committed by ). Changed prior to commit: https://reviews.llvm.org/D50960?vs=161442&id=161447#toc Repository: rCTE Clang Tools E

[PATCH] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

hokein added a comment. The interfaces look mostly good to me. Comment at: clangd/ClangdServer.cpp:73 + + void onPreambleAST(PathRef Path, ASTContext &Ctx, + std::shared_ptr PP) override { I think `Ctx` should be a `pointer` which gives us

[PATCH] D50889: [clangd] Make FileIndex aware of the main file

hokein added inline comments. Herald added a subscriber: kadircet. Comment at: clangd/index/FileIndex.h:70 + void + update(PathRef Path, ASTContext *AST, std::shared_ptr PP, + llvm::Optional> TopLevelDecls = llvm::None); Any strong reason to unify the i

[PATCH] D50961: [clangd] Simplify the code using UniqueStringSaver, NFC.

hokein closed this revision. hokein added a comment. committed in https://reviews.llvm.org/rL340161. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50961 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/

[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.

hokein added a comment. I think it is reasonable to make Sema support suggesting override methods, instead of implementing it in clangd side? Comment at: clangd/CodeComplete.cpp:206 + llvm::StringMap> Overrides; + for (auto *Method : dyn_cast(DC)->methods()) { +if (!Meth

[PATCH] D50966: Fix an undefined behavior when storing an empty StringRef.

hokein created this revision. hokein added a reviewer: ioeric. Herald added a subscriber: llvm-commits. Passing a nullptr to memcpy is UB. Repository: rL LLVM https://reviews.llvm.org/D50966 Files: lib/Support/StringSaver.cpp Index: lib/Support/StringSaver.cpp ===

[PATCH] D50967: [Preamble] Fix an undefined behavior when checking an empty preamble can be reused.

hokein created this revision. hokein added a reviewer: ilya-biryukov. Passing nullptr to memcmp is UB. Repository: rC Clang https://reviews.llvm.org/D50967 Files: lib/Frontend/PrecompiledPreamble.cpp Index: lib/Frontend/PrecompiledPreamble.cpp

[PATCH] D50966: Fix an undefined behavior when storing an empty StringRef.

hokein updated this revision to Diff 161467. hokein added a comment. Correct the fix. Repository: rL LLVM https://reviews.llvm.org/D50966 Files: lib/Support/StringSaver.cpp Index: lib/Support/StringSaver.cpp === --- lib/Supp

[PATCH] D50966: Fix an undefined behavior when storing an empty StringRef.

hokein added inline comments. Comment at: lib/Support/StringSaver.cpp:14 StringRef StringSaver::save(StringRef S) { + if (S.empty()) ioeric wrote: > Is it possible to reuse `StringRef::copy(Allocator &)` here? Unfortunately, not, `StringRef::copy` will change

[PATCH] D50966: Fix an undefined behavior when storing an empty StringRef.

This revision was automatically updated to reflect the committed changes. Closed by commit rL340170: Fix an undefined behavior when storing an empty StringRef. (authored by hokein, committed by ). Repository: rL LLVM https://reviews.llvm.org/D50966 Files: llvm/trunk/lib/Support/StringSaver.

[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

hokein added inline comments. Comment at: clang-tidy/abseil/NoInternalDepsCheck.cpp:20 + +bool IsInAbseilFile(const SourceManager& manager, SourceLocation loc){ + if (loc.isInvalid()) { hugoeg wrote: > hokein wrote: > > I think we can make it as an ASTMatcher in

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

hokein added inline comments. Comment at: clang-tidy/abseil/AbseilMatcher.h:16 + +/// Matches AST nodes that were expanded within abseil-header-files. +AST_POLYMORPHIC_MATCHER(isExpansionNotInAbseilHeader, nit: We need proper documentation for this matcher, since

[PATCH] D50862: [clang-tidy] Abseil: faster strsplit delimiter check

hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. The patch looks good. I'll commit for you once @JonasToth has no further comments. https://reviews.llvm.org/D50862 ___ cfe-commits mailing list

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

hokein added inline comments. Comment at: clang-tidy/abseil/AbseilMatcher.h:32 + auto Filename = FileEntry->getName(); + llvm::Regex RE("absl/(base|container|debugging|memory|meta|numeric|strings|" + "synchronization|types|utiliy)"); lebedev.ri

[PATCH] D50542: [clang-tidy] Add abseil-no-internal-deps check

hokein added a subscriber: deannagarcia. hokein added a comment. Your patch seems have some comments unaddressed but you marked done. About the abseil matcher stuff, you and @deannagarcia have an overlap, maybe just wait for https://reviews.llvm.org/D50580 to submit, and reuse the matcher in th

[PATCH] D51041: [clang-tidy] Don't run misc-unused-using-decls check in C++17.

hokein created this revision. hokein added a reviewer: ilya-biryukov. Herald added a subscriber: xazax.hun. It introduces some false positives which are non-trivial to fix. Ignore running the check in C++17. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51041 Files: clang-ti

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

hokein added a comment. Thanks for the updates. Looks mostly good, a few nits. Comment at: clang-tidy/abseil/AbseilMatcher.h:32 + +AST_POLYMORPHIC_MATCHER(isExpansionInAbseilHeader, +AST_POLYMORPHIC_SUPPORTED_TYPES( nit: this matcher nam

[PATCH] D50967: [Preamble] Fix an undefined behavior when checking an empty preamble can be reused.

hokein updated this revision to Diff 161891. hokein added a comment. Use the std::equal to replace the memcmp. Repository: rC Clang https://reviews.llvm.org/D50967 Files: lib/Frontend/PrecompiledPreamble.cpp Index: lib/Frontend/PrecompiledPreamble.cpp

[PATCH] D51041: [clang-tidy] Don't run misc-unused-using-decls check in C++17.

hokein updated this revision to Diff 161893. hokein added a comment. Remove redundant CPlusPlus2a. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51041 Files: clang-tidy/misc/UnusedUsingDeclsCheck.cpp test/clang-tidy/misc-unused-using-decls-cxx17.cpp Index: test/clang-tid

[PATCH] D51041: [clang-tidy] Don't run misc-unused-using-decls check in C++17.

hokein marked an inline comment as done. hokein added inline comments. Comment at: clang-tidy/misc/UnusedUsingDeclsCheck.cpp:35 + // it is not ture in C++17's template argument deduction. + if (!getLangOpts().CPlusPlus || getLangOpts().CPlusPlus17 || + getLangOpts().CPlusP

[PATCH] D51061: [clang-tidy] abseil-str-cat-append

hokein added a comment. Please make sure the code follows the LLVM code style, e.g. the variable name format "CamelName". Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D51061 ___ cfe-commits mailing list cfe-commits@lists.llvm.org

[PATCH] D50847: [clangd] Add callbacks on parsed AST in addition to parsed preambles

hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. Thanks! Looks good. Comment at: clangd/ClangdServer.cpp:73 + + void onPreambleAST(PathRef Path, ASTContext &Ctx, + std::shared_ptr PP) override { ---

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

hokein updated this revision to Diff 161927. hokein added a comment. Herald added a subscriber: kadircet. Update the patch based on our offline discussion - only one single clang intefaces implementation, and move finding references to current symbol collector; - store references in SymbolSlab;

[PATCH] D50967: [Preamble] Fix an undefined behavior when checking an empty preamble can be reused.

This revision was automatically updated to reflect the committed changes. Closed by commit rL340403: [Preamble] Fix an undefined behavior when checking an empty preamble can be… (authored by hokein, committed by ). Herald added a subscriber: llvm-commits. Repository: rL LLVM https://reviews.ll

[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.

hokein added a comment. Looks good mostly, a few nits. We should make sure all related comments are updated accordingly Comment at: clangd/CodeComplete.cpp:198 +static std::vector +getVirtualNonOverridenMethods(const DeclContext *DC, Sema *S) { + const auto *CR = llvm::dyn_ca

[PATCH] D51100: [clang-tidy] Add Abseil prefix to documentation

hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. LGTM. https://reviews.llvm.org/D51100 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-

[PATCH] D50862: [clang-tidy] Abseil: faster strsplit delimiter check

hokein closed this revision. hokein added a comment. Committed in https://reviews.llvm.org/rL340411. https://reviews.llvm.org/D50862 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D51100: [clang-tidy] Add Abseil prefix to documentation

This revision was automatically updated to reflect the committed changes. Closed by commit rL340412: [clang-tidy] Add Abseil prefix to documentation (authored by hokein, committed by ). Herald added a subscriber: llvm-commits. Changed prior to commit: https://reviews.llvm.org/D51100?vs=161940&i

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

hokein updated this revision to Diff 161962. hokein marked 6 inline comments as done. hokein added a comment. Address review comments. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50385 Files: clangd/index/FileIndex.cpp clangd/index/Index.cpp clangd/index/Index.h clan

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

hokein updated this revision to Diff 161972. hokein marked an inline comment as done. hokein added a comment. Add one more comment. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50385 Files: clangd/index/FileIndex.cpp clangd/index/Index.cpp clangd/index/Index.h clangd/

[PATCH] D50385: [clangd] Collect symbol occurrences from AST.

hokein added inline comments. Comment at: clangd/index/Index.h:46 + +inline bool operator==(const SymbolLocation::Position &L, + const SymbolLocation::Position &R) { ilya-biryukov wrote: > NIT: having friend decls inside the classes themselv

[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.

hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. Thanks, LGTM. Comment at: clangd/CodeComplete.cpp:210 +const std::string Name = Method->getNameAsString(); +Overrides[Name].push_back(Method); + } n

[PATCH] D50580: [clang-tidy] Abseil: no namespace check

hokein accepted this revision. hokein added a comment. This revision is now accepted and ready to land. Looks good. Comment at: test/clang-tidy/abseil-no-namespace.cpp:10 +#include "absl/external-file.h" +// CHECK: absl/external-file.h:1:11: warning: namespace 'absl' is reserve

[PATCH] D50898: [clangd] Suggest code-completions for overriding base class virtual methods.

hokein accepted this revision. hokein added a comment. still LG. Repository: rCTE Clang Tools Extra https://reviews.llvm.org/D50898 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D46684: [Frontend] Don't skip function body when the return type is dependent on the template parameter.

hokein created this revision. hokein added a reviewer: ilya-biryukov. Otherwise clang will provide an unexpected diagnostic error. Repository: rC Clang https://reviews.llvm.org/D46684 Files: lib/Sema/SemaDecl.cpp test/Index/skipped-auto-fn-templates.cpp Index: test/Index/skipped-auto-f

[PATCH] D38402: [clang-refactor] Apply source replacements

hokein added inline comments. Comment at: include/clang/Frontend/CommandLineSourceLoc.h:57 + std::string FileName; + std::pair Begin; + std::pair End; Add a comment documenting what the first element and second element of the pair represent for (? If I unders

[PATCH] D38723: [clang-rename] Don't add prefix qualifiers to the declaration and definition of the renamed symbol.

hokein created this revision. Herald added a subscriber: klimek. https://reviews.llvm.org/D38723 Files: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp unittests/Rename/RenameClassTest.cpp Index: unittests/Rename/RenameClassTest.cpp ===

[PATCH] D38402: [clang-refactor] Apply source replacements

hokein accepted this revision. hokein added a comment. LGTM, let's check in it. Repository: rL LLVM https://reviews.llvm.org/D38402 ___ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D38772: [refactor] allow the use of refactoring diagnostics

hokein added a comment. The code looks most good to me, a few nits. Comment at: lib/Basic/DiagnosticIDs.cpp:46 unsigned WarnShowInSystemHeader : 1; - unsigned Category : 5; + unsigned Category : 6; just curious: is this change needed?

[PATCH] D38723: [clang-rename] Don't add prefix qualifiers to the declaration and definition of the renamed symbol.

hokein updated this revision to Diff 118573. hokein marked 3 inline comments as done. hokein added a comment. Address review comments. https://reviews.llvm.org/D38723 Files: lib/Tooling/Refactoring/Rename/USRLocFinder.cpp unittests/Rename/RenameClassTest.cpp Index: unittests/Rename/RenameC

[PATCH] D38723: [clang-rename] Don't add prefix qualifiers to the declaration and definition of the renamed symbol.

This revision was automatically updated to reflect the committed changes. Closed by commit rL315452: [clang-rename] Don't add prefix qualifiers to the declaration and definition of… (authored by hokein). Changed prior to commit: https://reviews.llvm.org/D38723?vs=118573&id=118587#toc Repositor

[PATCH] D37856: [refactor] add support for refactoring options

hokein added a comment. Sorry for the delay. I saw you have reverted this commit somehow. A post commit. Comment at: cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp:113 Rules.push_back(createRefactoringActionRule( -SymbolSelectionRequirement())); +

[PATCH] D37856: [refactor] add support for refactoring options

hokein added inline comments. Comment at: cfe/trunk/lib/Tooling/Refactoring/Rename/RenamingAction.cpp:113 Rules.push_back(createRefactoringActionRule( -SymbolSelectionRequirement())); +SymbolSelectionRequirement(), OptionRequirement())); return Rules; --

  1   2   3   4   5   6   7   8   9   10   >