[PATCH] D156704: [clang][HeaderSearch] Treat framework headers as System for suggestPath

kadircet added inline comments. Comment at: clang/lib/Lex/HeaderSearch.cpp:1939 std::string HeaderSearch::suggestPathToFileForDiagnostics( llvm::StringRef File, llvm::StringRef WorkingDir, llvm::StringRef MainFile, sorry I guess my suggestion get distorte

[PATCH] D157071: [clangd] Dont assert on specific uris for diagnostics docs

kadircet created this revision. kadircet added a reviewer: hokein. Herald added a subscriber: arphaman. Herald added a project: All. kadircet requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-extra. To enable custo

[PATCH] D157078: [include-cleaner] Handle files with unnamed buffers

kadircet created this revision. kadircet added a reviewer: hokein. Herald added a subscriber: ChuanqiXu. Herald added a project: All. kadircet requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Some tools can register virtual buffe

[PATCH] D157071: [clangd] Dont assert on specific uris for diagnostics docs

This revision was automatically updated to reflect the committed changes. Closed by commit rG734da23e21eb: [clangd] Dont assert on specific uris for diagnostics docs (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157071/new/ h

[PATCH] D157078: [include-cleaner] Handle files with unnamed buffers

This revision was automatically updated to reflect the committed changes. Closed by commit rG31873d3fca38: [include-cleaner] Handle files with unnamed buffers (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157078/new/ https://

[PATCH] D157207: [clangd] Fix typo in comment

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. Thanks! LMK if i should land this for you (and an email address to set as commit author) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D15720

[PATCH] D157390: [clang-tidy][include-cleaner] Add option to control deduplication of findings per symbol

kadircet created this revision. kadircet added a reviewer: VitaNuo. Herald added subscribers: PiotrZSL, carlosgalvezp, xazax.hun. Herald added a reviewer: njames93. Herald added a project: All. kadircet requested review of this revision. Herald added a project: clang-tools-extra. Herald added a sub

[PATCH] D157390: [clang-tidy][include-cleaner] Add option to control deduplication of findings per symbol

kadircet updated this revision to Diff 548190. kadircet marked 2 inline comments as done. kadircet added a comment. Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157390/new/ https://reviews.llvm.org/D157390 Files: clang-tools-ex

[PATCH] D157390: [clang-tidy][include-cleaner] Add option to control deduplication of findings per symbol

kadircet updated this revision to Diff 548199. kadircet added a comment. - Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157390/new/ https://reviews.llvm.org/D157390 Files: clang-tools-extra/clang-tidy/misc/IncludeCleanerCheck.cpp clang

[PATCH] D157390: [clang-tidy][include-cleaner] Add option to control deduplication of findings per symbol

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG89d0a76be68b: [clang-tidy][include-cleaner] Add option to control deduplication of findings… (authored by kadircet). Repository: rG LLVM Github Mo

[PATCH] D157390: [clang-tidy][include-cleaner] Add option to control deduplication of findings per symbol

kadircet marked 2 inline comments as done. kadircet added a comment. hi @PiotrZSL, addressed those in 724b40a1379f7c116473a02a3cec4d341c475b46 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews

[PATCH] D157400: [include-cleaner] Dont boost private headers beyond public ones

kadircet created this revision. kadircet added a reviewer: VitaNuo. Herald added a project: All. kadircet requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Private headers should be the last resort, even if they match the name of

[PATCH] D157400: [include-cleaner] Dont boost private headers beyond public ones

This revision was automatically updated to reflect the committed changes. Closed by commit rG0a315be2a46f: [include-cleaner] Dont boost private headers beyond public ones (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D157400/ne

[PATCH] D157395: [include-cleaner] Follow `IWYU pragma: export` links transitively.

kadircet added a comment. i think we should also have some unittests in FindHeadersTest, some sample scenarios: foo.h #pragma once void foo(); export1.h #pragma once // IWYU pragma: private, include "public1.h" #include "foo.h" // IWYU pragma: export export2.h #pragma once //

[PATCH] D156704: [clang][HeaderSearch] Treat framework headers as System for suggestPath

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156704/new/ https://reviews.llvm.org/D156704

[PATCH] D157395: [include-cleaner] Follow `IWYU pragma: export` links transitively.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. > Order of exporters is from outermost to innermost. FWIW, that's mostly a coincidence. all 3 exporters are private headers, hence they're penalized for it. Afterwards "export1.h" is actua

[PATCH] D154503: [Sema] Fix handling of functions that hide classes

kadircet added a comment. Hi! After this patch clang seems to be crashing on: class a { static c a; void b(){a}; }; with stack trace and assertion error: $ ~/repos/llvm/build/bin/clang -xc++ repro.cc repro.cc:2:10: error: unknown type name 'c' 2 | static c a; |

[PATCH] D154503: [Sema] Fix handling of functions that hide classes

kadircet added a comment. In D154503#4576442 , @aaron.ballman wrote: > I'm not opposed to a revert if this is causing problems for your downstream, > but be sure to also remove the changes from the 17.x branch if you go this > route rather than fixing

[PATCH] D156659: [clangd] Rollforward include-cleaner library usage in symbol collector.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D156659/new/ https://reviews.llvm.org/D156659

[PATCH] D153769: [clangd] Implement the 'Organize Imports' source action. Fix include-cleaner findings in batch.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:752 auto Action = [File = File.str(), Sel, TweakID = TweakID.str(), - CB = std::move(CB), + CB = std::move(CB), ActionKinds, this](Expected I

[PATCH] D153769: [clangd] Implement the 'Organize Imports' source action. Fix include-cleaner findings in batch.

kadircet added a comment. FYI, i don't think you uploaded the new version of the patch Comment at: clang-tools-extra/clangd/ClangdServer.cpp:752 auto Action = [File = File.str(), Sel, TweakID = TweakID.str(), - CB = std::move(CB), + CB = std::

[PATCH] D154335: [clang][tooling] Fix early termination when there are nested expansions

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added a subscriber: arphaman. Herald added a project: All. kadircet requested review of this revision. Herald added projects: clang, clang-tools-extra. Herald added a subscriber: cfe-commits. This also does some cleanups,

[PATCH] D154349: [include-cleaner] Add a signal to down-rank exporting headers

kadircet created this revision. kadircet added a reviewer: hokein. Herald added a project: All. kadircet requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Currently exporter can have same relevance signals as the origin header whe

[PATCH] D154335: [clang][tooling] Fix early termination when there are nested expansions

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG9841daf27076: [clang][tooling] Fix early termination when there are nested expansions (authored by kadircet). Repository: rG LLVM Github Monorepo

[PATCH] D153340: [include-cleaner] Add an IgnoreHeaders flag to the command-line tool.

kadircet added inline comments. Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:67 /// Determine which headers should be inserted or removed from the main file. /// This exposes conclusions but not reasons: use lower-level walkUsed for t

[PATCH] D154443: [clangd] Downgrade deprecated warnings to hints

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added a subscriber: arphaman. Herald added a project: All. kadircet requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-extra. This tries t

[PATCH] D154443: [clangd] Downgrade deprecated warnings to hints

This revision was automatically updated to reflect the committed changes. Closed by commit rGa59b24be47ed: [clangd] Downgrade deprecated warnings to hints (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154443/new/ https://revi

[PATCH] D154473: [clang][Tooling] Add mapping for make_error_code

kadircet created this revision. kadircet added a reviewer: hokein. Herald added a project: All. kadircet requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D154473 Files: clang

[PATCH] D154473: [clang][Tooling] Add mapping for make_error_code

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG80c679220038: [clang][Tooling] Add mapping for make_error_code (authored by kadircet). Changed prior to commit: https://reviews.llvm.org/D154473?v

[PATCH] D153340: [include-cleaner] Add an IgnoreHeaders flag to the command-line tool.

kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:138 + /// Returns the path (without surrounding quotes/brackets) for the header.

[PATCH] D154477: [include-cleaner] Ignore the layering-violation errors for the standalone tool

kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/include-cleaner/tool/IncludeCleaner.cpp:99 + bool BeginInvocation(CompilerInstance &CI) override { +// Disable the clang-module-bas

[PATCH] D154349: [include-cleaner] Add a signal to down-rank exporting headers

This revision was automatically updated to reflect the committed changes. Closed by commit rG5933d265b72a: [include-cleaner] Add a signal to down-rank exporting headers (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D154349/new/

[PATCH] D152900: [clangd] Update symbol collector to use include-cleaner.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:85 auto Task = [FIndex(FIndex), Path(Path.str()), Version(Version.str()), - ASTCtx(std::move(ASTCtx)), - CanonIncludes(CanonIncludes)]() mutable { +

[PATCH] D154950: [include-cleaner] Fix the `fixIncludes` API not respect main-file header.

kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:303 - EXPECT_EQ(fixIncludes(Results, Code, format::getLLVMStyle()), R"cpp( + EXPECT_EQ(fi

[PATCH] D154962: [clangd] Use canonical path as resolved path for includes.

kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/Headers.cpp:57-61 + auto CanonicalPath = + File ? getCanonicalPath(File->getFileEntry().getLastRef(), +

[PATCH] D145302: [clangd] Add library for clangd main function

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145302/new/ https://reviews.llvm.org/D145302 _

[PATCH] D154963: [Format][Tooling] Fix HeaderIncludes::insert not respect the main-file header.

kadircet added inline comments. Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:338 int Priority = Categories.getIncludePriority( -CurInclude.Name, /*CheckMainHeader=*/FirstIncludeOffset < 0); +CurInclude.Name, /*CheckMainHeader=*/true); Cate

[PATCH] D154963: [Format][Tooling] Fix HeaderIncludes::insert not respect the main-file header.

kadircet added inline comments. Comment at: clang/lib/Tooling/Inclusions/HeaderIncludes.cpp:369-370 + QuotedName, /*CheckMainHeader=*/!MainIncludeFound); + if (Priority == 0) +MainIncludeFound = true; auto CatOffset = CategoryEndOffsets.find(Priority); --

[PATCH] D155195: [include-cleaner] Avoid a caching issue when running --edit mode on multiple files.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D155195/new/ https://reviews.llvm.org/D155195

[PATCH] D155173: [clangd] Refine the workflow for diagnostic Fixits.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/ClangdServer.cpp:671 + // FIMXE: this is tricky + llvm::StringRef(LSPDiag.Message) + .starts_with_insensitive(Diag.Message)) this is tricky indeed and

[PATCH] D155215: [clangd] Fix the range for include reference to itself.

kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/IncludeCleaner.h:87 + +// Returns the range starting at '#' and ending at EOL. Escaped newlines are not +// handled. ---

[PATCH] D158515: [include-cleaner] Make handling of enum constants similar to members

This revision was automatically updated to reflect the committed changes. kadircet marked 2 inline comments as done. Closed by commit rGab090e9e49ff: [include-cleaner] Make handling of enum constants similar to members (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE L

[PATCH] D157963: [clang-format] Annotate constructor/destructor names

kadircet added a comment. In D157963#4621206 , @HazardyKnusperkeks wrote: > @kadircet shouldn't you at least say why it got reverted? Even better state > your problem and give a chance to fix before you revert? Hi, https://reviews.llvm.org/rG7590b76570

[PATCH] D159337: [clangd][tests] Assert for idleness of scheduler

kadircet created this revision. kadircet added reviewers: sammccall, nridge. Herald added subscribers: arphaman, javed.absar. Herald added a project: All. kadircet requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-e

[PATCH] D159338: [clangd][tests] Bump timeouts in TUSchedulerTests to 60 secs

kadircet created this revision. kadircet added reviewers: sammccall, nridge. Herald added subscribers: arphaman, javed.absar. Herald added a project: All. kadircet requested review of this revision. Herald added subscribers: cfe-commits, MaskRay, ilya-biryukov. Herald added a project: clang-tools-e

[PATCH] D159337: [clangd][tests] Assert for idleness of scheduler

This revision was automatically updated to reflect the committed changes. Closed by commit rG69feef0e8277: [clangd][tests] Assert for idleness of scheduler (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159337/new/ https://rev

[PATCH] D159338: [clangd][tests] Bump timeouts in TUSchedulerTests to 60 secs

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG16b8b3e59f7e: [clangd][tests] Bump timeouts in TUSchedulerTests to 60 secs (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SI

[PATCH] D158566: Add CLANGD_INCLUDE_TESTS as a separate flag to control clangd tests

kadircet added a comment. In D158566#4616417 , @ilya-biryukov wrote: > Open question: I also feel like the best option here is to fix the tests, but > I'm not sure how hard that would be. @sammccall any thoughts? > I suspect the particular tests are fla

[PATCH] D159363: [clangd] SIGSEGV at clangd: DiagnosticConsumer Is Used After Free

kadircet added a comment. thanks, the fix LGTM as well. but i wonder how this surfaces, to make sure we're taking necessary precautions in the future. we definitely have a dangling reference, which isn't great. but it's surprising that we access diags consumer during indexing. I assume it's abo

[PATCH] D159363: [clangd] SIGSEGV at clangd: DiagnosticConsumer Is Used After Free

kadircet added a comment. thanks, the fix LGTM as well. but i wonder how this surfaces, to make sure we're taking necessary precautions in the future. we definitely have a dangling reference, which isn't great. but it's surprising that we access diags consumer during indexing. I assume it's abo

[PATCH] D156659: [clangd] Rollforward include-cleaner library usage in symbol collector.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:840 -if (S->Scope == "std::" && S->Name == "move") { - if (!S->Signature.contains(',')) -return ""; - return ""; -} - -if (auto StdSym = tooling::stdl

[PATCH] D159441: [include-cleaner] Weaken signal for boosting preferred headers

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added a project: All. kadircet requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Putting preferred header signal above completeness implied we would upra

[PATCH] D159263: [clang-tidy] misc-include-cleaner: avoid duplicated fixes

kadircet added a comment. Thanks a lot for the patch @danix800 ! I initially was rather focused on behavior of this check in workflows that require seeing "self-contained-diags", but also I rather see the bulk-apply use cases as always requiring clang-format afterwards. Hence didn't really try

[PATCH] D159263: [clang-tidy] misc-include-cleaner: avoid duplicated fixes

kadircet added a comment. In D159263#4638199 , @danix800 wrote: > If we have further plans from upstream on these checker/clang-format related > logic, I can wait and abandon this revision. Not at the moment, so please don't! > Or if such issue does ne

[PATCH] D159441: [include-cleaner] Weaken signal for boosting preferred headers

kadircet added a comment. see also https://github.com/llvm/llvm-project/issues/62172 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159441/new/ https://reviews.llvm.org/D159441 ___ cfe-commits mailing lis

[PATCH] D159441: [include-cleaner] Weaken signal for boosting preferred headers

This revision was automatically updated to reflect the committed changes. Closed by commit rG73b2c86d95dc: [include-cleaner] Weaken signal for boosting preferred headers (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D159441/new

[PATCH] D158566: Add CLANGD_INCLUDE_TESTS as a separate flag to control clangd tests

kadircet added a comment. hi @hiraditya , i believe your issues should disappear starting with 9a26d2c6d35f574d7a4b06a5a22f8a1c063cb664 . LMK if you're still facing problems and want to move forward with such a patch CHANGES

[PATCH] D157610: [include-cleaner][clangd][clang-tidy] Ignore resource dir during include-cleaner analysis.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks! Comment at: clang-tools-extra/clangd/unittests/IncludeCleanerTests.cpp:587 + TU.ExtraArgs.push_back(testPath("resources")); + TU.AdditionalFiles["resources/incl

[PATCH] D156659: [clangd] Rollforward include-cleaner library usage in symbol collector.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:912 +if (Directives & Symbol::Import) { + if (auto IncludeHeader = HeaderFileURIs->getIncludeHeader(FID); + !IncludeHeader.empty()) { VitaNuo wrote: >

[PATCH] D156659: [clangd] Rollforward include-cleaner library usage in symbol collector.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/index/SymbolCollector.cpp:828 - tooling::stdlib::Lang Lang = tooling::stdlib::Lang::CXX; -if (LangOpts.C11) - Lang = tooling::stdlib::Lang::C; sorry i got confused, this also works for

[PATCH] D137698: [include-cleaner] Add self-contained file support for PragmaIncludes.

kadircet added a comment. thanks, mostly LG a bunch of suggestions for tests Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:74 + /// Returns true if the given file is a self-contained file. + bool isSelfContained(const FileEntry *File) c

[PATCH] D137698: [include-cleaner] Add self-contained file support for PragmaIncludes.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks, lgtm! Comment at: clang-tools-extra/include-cleaner/unittests/FindHeadersTest.cpp:47 + } + void buildAST() { AST = std::make_unique(Inputs); } + ---

[PATCH] D138287: [clang][RISCV] Drop caching from RVVType as it introduces data races

kadircet created this revision. kadircet added reviewers: kito-cheng, ilya-biryukov. Herald added subscribers: sunshaoce, VincentWu, vkmr, frasercrmck, evandro, luismarques, apazos, sameer.abuasal, s.egerton, Jim, benna, psnobl, jocewei, PkmX, the_o, brucehoult, MartinMosbeck, rogfer01, edward-jo

[PATCH] D124730: [RISCV][NFC] Refactor RISC-V vector intrinsic utils.

kadircet added a comment. In D124730#3916786 , @kadircet wrote: > Hi @kito-cheng, can you please let us know if you're working on a fix here > (and whether it seems to be close)? otherwise i am planning to revert this > and consequent changes, as it's c

[PATCH] D138429: [clang][RISCV][NFC] Prevent data race in RVVType::computeType

kadircet added a comment. thanks! +1 from my side as well for thread-safety purposes (and I hope having 2 separate caches for tablegen and sema doesn't have unexpected effects). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138429/new/ https://rev

[PATCH] D138429: [clang][RISCV][NFC] Prevent data race in RVVType::computeType

kadircet added inline comments. Comment at: clang/include/clang/Support/RISCVVIntrinsicUtils.h:289 + + static uint64_t computeRVVTypeHashValue(BasicType BT, int Log2LMUL, + PrototypeDescriptor Proto); khchen wrote: > `sta

[PATCH] D138491: [clangd] Add script to maintain list of fast clang-tidy checks

kadircet added inline comments. Comment at: clang-tools-extra/clangd/TidyFastChecks.py:31 +default='clang-tools-extra/clangd/TidyFastChecks.inc') +parser.add_argument('--file', help='clangd binary to invoke', +default='clang/lib/Sema/Sema.cpp') `file to u

[PATCH] D138491: [clangd] Add script to maintain list of fast clang-tidy checks

kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/TidyFastChecks.inc:2 +// This file is generated, do not edit it directly! +// This describes +#ifndef FAST can

[PATCH] D138505: [clangd] Don't run slow clang-tidy checks

kadircet added a comment. i can't think of a proper way to test this out either. unless we somehow let slow-tidy-check list to be configurable, so probably fine to make sure it works locally and hope that new people introducing tidy checks do complain. Comment at: clang-tools

[PATCH] D138648: [include-cleaner] Make Symbol (and Macro) hashable.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks, lgtm! Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:147 + using Outer = clang::include_cleaner::Symbol; + constexpr stati

[PATCH] D138677: [Lex] Fix suggested spelling of /usr/bin/../include/foo

kadircet added inline comments. Comment at: clang/lib/Lex/HeaderSearch.cpp:1932 + + llvm::SmallString<32> FilePath(File.begin(), File.end()); + path::remove_dots(FilePath, /*remove_dot_dot=*/true); nit: you can change the method signature to take in a SmallSt

[PATCH] D138678: [include-cleaner] Capture private headers in PragmaIncludes.

kadircet added inline comments. Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:97 + /// the IWYU private pragmas with public mapping. + llvm::DenseSet IWYUPrivate; + i'd actually merge this with the previous map, and store

[PATCH] D138546: Clangd: Preserve target flags in system includes extractor

kadircet added a comment. Sorry for noticing this so late. Yes the initial reason we left `sysroot` and `std/builtin-inc` related flags were exactly that, in theory they could vary but in practice they were applied to all or none of the project. Regarding this change itself, surely preserving t

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

kadircet added inline comments. Comment at: clang-tools-extra/clangd/TidyProvider.cpp:215 "-bugprone-use-after-move", + // Using an CFG and might crash on invalid code: + "-performance-unnecessary-copy-on-last-us

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

kadircet added a comment. another thing that i noticed is usage of east consts in the implementation files. no one seem to have brought it up so far (at least none that i can see). in theory there are no explicit guidelines about it in LLVM coding style, but rest of the codebase uses west const

[PATCH] D138709: Reland "[Lex] Fix suggested spelling of /usr/bin/../include/foo"

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks! Comment at: clang/lib/Lex/HeaderSearch.cpp:1936 + path::remove_dots(FilePath, /*remove_dot_dot=*/true); + path::native(FilePath, path::Style::posix); + File =

[PATCH] D138678: [include-cleaner] Capture private headers in PragmaIncludes.

kadircet added inline comments. Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:237 + auto *FE = SM.getFileEntryForID(SM.getFileID(Range.getBegin())); + if (!FE) { +return false; nit: you can drop the braces here. LLVM convention is

[PATCH] D137205: [clang-tidy] Add performance-unnecessary-copy-on-last-use check

kadircet added a comment. In D137205#3951230 , @Febbe wrote: > In D137205#3950722 , @kadircet > wrote: > >> another thing that i noticed is usage of east consts in the implementation >> files. no one seem to hav

[PATCH] D138779: [include-cleaner] Filter out references that not spelled in the main file.

kadircet added a comment. Maybe I am missing some context here, but why are we trying to filter out references outside of the main file? ATM there's no concept of main file in neither walkUsed, it just receives some ast nodes for analyzing references to declarations inside these nodes, and the

[PATCH] D138505: [clangd] Don't run slow clang-tidy checks by default

kadircet added a comment. thanks LG, i'd like to hear how we're planning to let downstream users customise the list of fast checks. otherwise they have to run with `Loose` at all times. the easiest i can think of is, generating their own `fastchecks.inc` fragment and #include that in addition t

[PATCH] D135508: [clangd] Heuristic to avoid desync if editors are confused about newline-at-eof

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. as discussed, this is a rather contained fix that are likely to help with other editors that might get confused with new-lines at EOF. so LGTM Repository: rG LLVM Github Monorepo CHANG

[PATCH] D135953: [IncludeCleaner] Introduce decl to location mapping

kadircet updated this revision to Diff 478611. kadircet marked 6 inline comments as done. kadircet added a comment. - Leaving out all the pieces around signals as discussed. - Update tests to use a LocateExample helper, have a test for macros. - Get rid of residues in other test files. Repositor

[PATCH] D138200: [include-cleaner] Make use of locateSymbol in WalkUsed

kadircet updated this revision to Diff 478614. kadircet added a comment. - Rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138200/new/ https://reviews.llvm.org/D138200 Files: clang-tools-extra/include-cleaner/lib/Analysis.cpp clang-tools-

[PATCH] D138821: [include-cleaner] Remove filtering from UsingDecl visit.

kadircet added inline comments. Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:86 report(UD->getLocation(), TD, IsUsed ? RefType::Explicit : RefType::Ambiguous); } hokein wrote: > we should report all references as explici

[PATCH] D128457: [clangd] Add new IncludeType to IncludeHeaderWithReferences

kadircet added inline comments. Comment at: clang-tools-extra/clangd/Headers.cpp:237 for (const auto &Include : Includes) -Headers.push_back(Include.IncludeHeader); +if (Include.SupportedDirectives & clang::clangd::Symbol::Include) + Headers.push_back(Include.Incl

[PATCH] D139018: [include-cleaner] Record whether includes are spelled with quotes

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks, lgtm! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139018/new/ https://reviews.llvm.org/D139018 __

[PATCH] D135953: [IncludeCleaner] Introduce decl to location mapping

This revision was automatically updated to reflect the committed changes. kadircet marked 6 inline comments as done. Closed by commit rGf82f5b0507a2: [include-cleaner] Introduce symbol to location mapping (authored by kadircet). Changed prior to commit: https://reviews.llvm.org/D135953?vs=47861

[PATCH] D139093: [include-cleaner] Use RAV instead of ASTMatchers in LocateSymbolTest

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added a project: All. kadircet requested review of this revision. Herald added a reviewer: jdoerfert. Herald added subscribers: cfe-commits, sstefan1. Herald added a project: clang-tools-extra. ASTMatchers are pulling in

[PATCH] D139093: [include-cleaner] Use RAV instead of ASTMatchers in LocateSymbolTest

kadircet marked an inline comment as done. kadircet added inline comments. Comment at: clang-tools-extra/include-cleaner/unittests/LocateSymbolTest.cpp:60 + return true; +Out = ND; +return false; sammccall wrote: > you might want to EXPEC

[PATCH] D139093: [include-cleaner] Use RAV instead of ASTMatchers in LocateSymbolTest

This revision was automatically updated to reflect the committed changes. kadircet marked an inline comment as done. Closed by commit rG4764ee6055c7: [include-cleaner] Use RAV instead of ASTMatchers in LocateSymbolTest (authored by kadircet). Changed prior to commit: https://reviews.llvm.org/D1

[PATCH] D139013: [include-cleaner] clang-include-cleaner can print/apply edits

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks! Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:65 +llvm::ArrayRef MacroRefs, +

[PATCH] D128677: [clangd] Add support for generating #import edits

kadircet added a comment. we should have tests in `clang/unittests/Tooling/HeaderIncludesTest.cpp` and the commit itself should be tagged as `[clang][Tooling]` rather than `[clangd]`. Comment at: clang-tools-extra/clangd/Headers.h:250 + llvm::Optional insert(llvm::StringRef V

[PATCH] D139087: [include-cleaner] Handle base class member access from derived class.

kadircet added inline comments. Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:62 bool VisitMemberExpr(MemberExpr *E) { -report(E->getMemberLoc(), E->getFoundDecl().getDecl()); +// Instead of the FieldDecl for MemberExpr, we report the Decl of +// th

[PATCH] D139013: [include-cleaner] clang-include-cleaner can print/apply edits

kadircet accepted this revision. kadircet added a comment. thanks! Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:77 + bool Satisfied = false; + for (const Header &H : Providers) { + if (H.kind() == Header::Physical && H.phy

[PATCH] D76304: [clangd] Update TUStatus api to accommodate preamble thread

kadircet updated this revision to Diff 252248. kadircet marked 14 inline comments as done. kadircet added a comment. - Address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76304/new/ https://reviews.llvm.org/D76304 Files: clang-tools-e

[PATCH] D76304: [clangd] Update TUStatus api to accommodate preamble thread

kadircet added inline comments. Comment at: clang-tools-extra/clangd/TUScheduler.cpp:277 build(std::move(*CurrentReq)); + bool IsEmpty = false; + { sammccall wrote: > kadircet wrote: > > sammccall wrote: > > > Seems clearer to do this immediately

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

kadircet created this revision. kadircet added a reviewer: sammccall. Herald added subscribers: cfe-commits, usaxena95, arphaman, jkorous, MaskRay, javed.absar, ilya-biryukov. Herald added a project: clang. kadircet added a parent revision: D76304: [clangd] Update TUStatus api to accommodate prea

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

kadircet updated this revision to Diff 252633. kadircet added a comment. - Build golden asts in ASTWorker thread, rather than preamble thread. - Explain new model in file comments. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D76725/new/ https://re

[PATCH] D76725: [clangd] Build ASTs only with fresh preambles or after building a new preamble

kadircet added a comment. In D76725#1940282 , @sammccall wrote: > So I think we need a careful description of the new model somewhere. Not so > much on specific changes/constraints parts of the code operate under, but > what it's trying to do. > > My bes

[PATCH] D76801: [AST] Print a> without extra spaces in C++11 or later.

kadircet accepted this revision. kadircet marked an inline comment as done. kadircet added a comment. This revision is now accepted and ready to land. LGTM, thanks ! Comment at: clang/unittests/AST/DeclPrinterTest.cpp:1161 "A", -"Z > A")); -// Should be: with semic

<    24   25   26   27   28   29   30   31   32   33   >