[PATCH] D145843: [clangd] Add option to always insert headers with <> instead of ""

kadircet added a comment. sorry but I am not sure what's the value proposed by this patch in its current form. in https://github.com/clangd/clangd/issues/1247 and other places we've discussed this, i believe the sentiment was towards providing a config option that'll let people customize

[PATCH] D146417: [clangd] Fix AddUsing in the face of typo-correction

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG6f23fee4ef98: [clangd] Fix AddUsing in the face of typo-correction (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE

[PATCH] D146406: [IncludeCleaner][clangd] Mark umbrella headers as users of private

kadircet added inline comments. Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:100 + // Check if main file is the public interface for a private header. If so + // we shouldn't diagnose it as unused. + if (auto PHeader = PI->getPublic(I.Resolved);

[PATCH] D146279: [clangd] Extend CollectMainFileMacros.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks! Comment at: clang-tools-extra/clangd/CollectMacros.h:48 public: - explicit CollectMainFileMacros(const SourceManager , MainFileMacros ) - : SM(SM),

[PATCH] D146417: [clangd] Fix AddUsing in the face of typo-correction

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. Fixes

[PATCH] D146406: [IncludeCleaner][clangd] Mark umbrella headers as users of private

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 projects: clang, clang-tools-extra. Private

[PATCH] D143755: [clang-format] Add a space between an overloaded operator and '>'

kadircet added a comment. Hi @owenpan, this seems to be crashing for: struct Foo { operator enum foo{} }; with stack trace: $ ~/repos/llvm/build/bin/clang-format format_crash.cc --dry-run clang-format: /usr/local/google/home/kadircet/repos/llvm/clang/lib/Format/TokenAnnotator.cpp:1229:

[PATCH] D146202: [clang] Fix a UsingTemplate regression after 3e78fa860235431323aaf08c8fa922d75a7cfffa

kadircet accepted this revision. kadircet added a subscriber: ChuanqiXu. kadircet added a comment. This revision is now accepted and ready to land. FWIW, I believe this patch does the right thing by marking the `DeducedTemplateSpecializationType` as `using`. It's explicitly introduced into the

[PATCH] D146116: [clangd] Respect WantDiags when emitting diags from possibly stale preambles

This revision was automatically updated to reflect the committed changes. Closed by commit rG9e8bac748064: [clangd] Respect WantDiags when emitting diags from possibly stale preambles (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D146116: [clangd] Respect WantDiags when emitting diags from possibly stale preambles

kadircet created this revision. kadircet added a reviewer: hokein. 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-extra.

[PATCH] D146028: [clangd] Patch main file macros in preamble

This revision was automatically updated to reflect the committed changes. Closed by commit rG82c8bf8fcc91: [clangd] Patch main file macros in preamble (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146028/new/

[PATCH] D146026: [clangd] Patch PragmaMarks in preamble section of the file

This revision was automatically updated to reflect the committed changes. Closed by commit rG9c888120e3c8: [clangd] Patch PragmaMarks in preamble section of the file (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146026/new/

[PATCH] D146028: [clangd] Patch main file macros in preamble

kadircet updated this revision to Diff 505093. kadircet marked an inline comment as done. kadircet added a comment. - Rebase - Prevent unnecessary copies Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146028/new/ https://reviews.llvm.org/D146028

[PATCH] D146026: [clangd] Patch PragmaMarks in preamble section of the file

kadircet added inline comments. Comment at: clang-tools-extra/clangd/Preamble.cpp:576 + +std::vector getPragmaMarks(const ScannedPreamble ) { + std::vector Marks; hokein wrote: > any reason not calling `collectPragmaMarksCallback` just like the >

[PATCH] D146026: [clangd] Patch PragmaMarks in preamble section of the file

kadircet updated this revision to Diff 505091. kadircet marked an inline comment as done. kadircet added a comment. - Use collectPragmaMarksCallback Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D146026/new/ https://reviews.llvm.org/D146026 Files:

[PATCH] D146024: [clangd] Drop stale macro and mark ranges

This revision was automatically updated to reflect the committed changes. kadircet marked an inline comment as done. Closed by commit rGc11c2f5f6548: [clangd] Drop stale macro and mark ranges (authored by kadircet). Changed prior to commit: https://reviews.llvm.org/D146024?vs=504990=505046#toc

[PATCH] D146021: [Tooling/Inclusion] Index more sub std namespace symbols.

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/D146021/new/ https://reviews.llvm.org/D146021

[PATCH] D146028: [clangd] Patch main file macros in preamble

kadircet created this revision. 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. Depends on D146026

[PATCH] D146026: [clangd] Patch PragmaMarks in preamble section of the file

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. Repository:

[PATCH] D146024: [clangd] Drop stale macro and mark ranges

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. I'll follow up

[PATCH] D145921: [clangd] Add missing unittests to build graph

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGe26dad0a661e: [clangd] Add missing unittests to build graph (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D143436: [clangd] Move standard options adaptor to CommandMangler

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks, lgtm! Comment at: clang-tools-extra/clangd/CompileCommands.cpp:199 + // use/change working directory, which ExpandResponseFiles doesn't). + FS =

[PATCH] D145921: [clangd] Add missing unittests to build graph

kadircet marked an inline comment as done. kadircet added inline comments. Comment at: clang-tools-extra/clangd/unittests/tweaks/SpecialMembersTests.cpp:21 EXPECT_UNAVAILABLE("struct S { ^ };"); - EXPECT_UNAVAILABLE("union ^U {};"); + EXPECT_AVAILABLE("union ^U {};");

[PATCH] D145921: [clangd] Add missing unittests to build graph

kadircet updated this revision to Diff 504597. kadircet marked an inline comment as done. kadircet added a comment. - Disable special members tweaks on unions Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D145921/new/

[PATCH] D127184: [clangd] Add to header map

kadircet added a comment. we can also handle them through the stdlib symbol mappings, see https://github.com/llvm/llvm-project/issues/61373 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D127184/new/ https://reviews.llvm.org/D127184

[PATCH] D145921: [clangd] Add missing unittests to build graph

kadircet created this revision. kadircet added reviewers: sammccall, 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. Also

[PATCH] D145773: [clangd] UnusedIncludes: Strict config now uses the include-cleaner-library implementation.

kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:438 + .map("Experiment", +

[PATCH] D145776: [clangd] Remove the classical clangd-own unsued-include implementation.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. i'd still merge this with the previous patch, as all of this is dead code after config option deletion. so it'd be better to just revert a single patch if we want to restore the old

[PATCH] D145773: [clangd] UnusedIncludes: Strict config now uses the include-cleaner-library implementation.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:438 + .map("Experiment", + Config::IncludesPolicy::Strict) // for backward +

[PATCH] D143436: [clangd] Move standard options adaptor to CommandMangler

kadircet added a comment. > @kadircet it is obvious that there is something in this diff that causes this > hesitancy in accepting it. I'm ready to keep iterating on the solution but I > need a clue what needs the improvement. Please comment. sorry for the late reply, i was on vacation and

[PATCH] D145553: [Tooling/Inclusion] Add missing placerholder _1 symbols.

kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Tooling/Inclusions/Stdlib/StdSpecialSymbolMap.inc:260 +// text, which are not handled by the script. +// N is an implementation-defined number,

[PATCH] D143496: [clangd] Add support for missing includes analysis.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks for bearing with me, let's ship it! Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:456 + tooling::IncludeStyle IncludeStyle; + auto DefaultStyle =

[PATCH] D143496: [clangd] Add support for missing includes analysis.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:456 + auto IncludeStyle = + clang::format::getLLVMStyle().IncludeStyle; + auto DefaultStyle = clang::format::getStyle( creating a copy of LLVM style unnecessarily all

[PATCH] D145228: [clangd] Add clangd headers to install targets

kadircet requested changes to this revision. kadircet added a comment. This revision now requires changes to proceed. i agree with Sam's concerns here. clangd isn't designed to be consumed as a library, but rather as a binary through LSP. increasing surface are here and letting people build

[PATCH] D143496: [clangd] Add support for missing includes analysis.

kadircet added a comment. thanks, looks great! Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:451 + tooling::HeaderIncludes HeaderIncludes(AST.tuPath(), Code, + tooling::IncludeStyle{}); + const SourceManager =

[PATCH] D145365: [Tooling/Inclusion] Add missing index_sequence symbols.

kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Tooling/Inclusions/Stdlib/StdSpecialSymbolMap.inc:252 +// Remove them when the cppreference offline archive catches up. +SYMBOL(index_sequence,

[PATCH] D145365: [Tooling/Inclusion] Add missing index_sequence symbols.

kadircet added inline comments. Comment at: clang/lib/Tooling/Inclusions/Stdlib/StdSpecialSymbolMap.inc:252 +// Remove them when the cppreference offline archive catches up. +SYMBOL(index_sequence, std::, ) +SYMBOL(index_sequence_for, std::, ) there's also

[PATCH] D145364: [include-cleaner] Fix a crash on non-identifier-name symbols.

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/test/nocrash.cpp:1 +// RUN: clang-include-cleaner %s -- + hokein wrote: > I have considered

[PATCH] D144721: [Tooling/Inclusion] Add the generic abs symbol to the table.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. oh right, i thought these were also provided by multiple headers, but looks like they're only provided by `cstdlib`, thanks! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D144721: [Tooling/Inclusion] Add the generic abs symbol to the table.

kadircet added a comment. this fixes the `abs` in the mentioned issue, but leaves the friends out :( those are specifically `std::labs, std::llabs, std::imaxabs` Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144721/new/

[PATCH] D143496: [clangd] Add support for missing includes analysis.

kadircet added a comment. thanks! looks amazing, we're missing a little bit of test coverage though Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:280 +bool isFilteredByConfig(const Config , llvm::StringRef HeaderSpelling) { + // Convert the path to Unix slashes

[PATCH] D144713: [Tooling/Includsion] Add the missing NULL symbol to the table.

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/D144713/new/ https://reviews.llvm.org/D144713

[PATCH] D144713: [Tooling/Includsion] Add the missing NULL symbol to the table.

kadircet added inline comments. Comment at: clang/lib/Tooling/Inclusions/Stdlib/StdSpecialSymbolMap.inc:242 +// Macros +SYMBOL(NULL, , ) +SYMBOL(NULL, , ) we use `None` for global namespace rather than leaving empty. let's keep it that way? Repository: rG

[PATCH] D144646: [Tooling][Inclusions] Add c-header and global namespace alternatives for size_t

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG385c8cd3cd66: [Tooling][Inclusions] Add c-header and global namespace alternatives for size_t (authored by kadircet). Repository: rG LLVM Github

[PATCH] D144708: [clangd] Fix UB in scanPreamble

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. kadircet marked an inline comment as done. Closed by commit rGf393e1f6b3b4: [clangd] Fix UB in scanPreamble (authored by kadircet). Changed prior to commit:

[PATCH] D144708: [clangd] Fix UB in scanPreamble

kadircet marked an inline comment as done. kadircet added inline comments. Comment at: clang-tools-extra/clangd/Preamble.cpp:351 auto PreambleContents = - llvm::MemoryBuffer::getMemBufferCopy(Contents.substr(0, Bounds.Size)); +

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

kadircet added a comment. thanks for the ping! see the discussion in https://reviews.llvm.org/D142890#4149679 Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143096/new/ https://reviews.llvm.org/D143096

[PATCH] D142890: [clangd] Add config option for fast diagnostics mode

kadircet added a comment. In D142890#4149181 , @vitalybuka wrote: > One of your patches likely introduced UB > https://lab.llvm.org/buildbot/#/builders/85/builds/14558 > Can you please take a look? Thanks a lot Vitaly and sorry for not noticing this

[PATCH] D144708: [clangd] Fix UB in scanPreamble

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.

[PATCH] D144646: [Tooling][Inclusions] Add c-header and global namespace alternatives for size_t

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/D144646 Files:

[PATCH] D144641: [clangd] Set diag data before emitting

This revision was automatically updated to reflect the committed changes. Closed by commit rG981e3a35a145: [clangd] Set diag data before emitting (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144641/new/

[PATCH] D144641: [clangd] Set diag data before emitting

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. Also fixes a

[PATCH] D144599: [clangd/index/remote]NFC: Adapt code to newer grpc/protobuf versions

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks, indeed the protobuf version used by grpc-1.36.3 has these available,

[PATCH] D144582: [include-cleaner] Always treat constructor calls as implicit

This revision was automatically updated to reflect the committed changes. Closed by commit rG279b4985ed4f: [include-cleaner] Always treat constructor calls as implicit (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144582/new/

[PATCH] D143496: [clangd] Add support for missing includes analysis.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:291 } for (auto : Cfg.Diagnostics.Includes.IgnoreHeader) { // Convert the path to Unix slashes and try to match against the filter. can you also change the logic

[PATCH] D144579: [include-cleaner] Check macros against stdlib database

This revision was automatically updated to reflect the committed changes. Closed by commit rG961e32c587cb: [include-cleaner] Check macros against stdlib database (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144579/new/

[PATCH] D144582: [include-cleaner] Always treat constructor calls as implicit

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. Treating constructor calls when the type name isn't explicitly spelled can

[PATCH] D144579: [include-cleaner] Check macros against stdlib database

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. Repository: rG LLVM Github Monorepo https://reviews.llvm.org/D144579

[PATCH] D144456: [clangd] Publish diagnostics with stale preambles

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG465ee9bfb26d: [clangd] Publish diagnostics with stale preambles (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

This revision was automatically updated to reflect the committed changes. Closed by commit rG8c2a12f7f9b6: [clangd] Provide patched diagnostics with preamble patch (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143096/new/

[PATCH] D143095: [clangd] Respect preamble-patch when handling diags

This revision was automatically updated to reflect the committed changes. Closed by commit rG909cd1f9a893: [clangd] Respect preamble-patch when handling diags (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143095/new/

[PATCH] D142890: [clangd] Add config option for fast diagnostics mode

This revision was automatically updated to reflect the committed changes. kadircet marked an inline comment as done. Closed by commit rG7177a237b68f: [clangd] Add config option for fast diagnostics mode (authored by kadircet). Changed prior to commit:

[PATCH] D143093: [clangd] #undef macros inside preamble patch

This revision was automatically updated to reflect the committed changes. Closed by commit rG75ae784e8f49: [clangd] #undef macros inside preamble patch (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143093/new/

[PATCH] D144484: [Tooling/Inclusion] Handle std::get symbol.

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/D144484/new/ https://reviews.llvm.org/D144484

[PATCH] D143509: Move the BySpelling map to IncludeStructure.

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks (both for the patch and for bearing with me)! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143509/new/

[PATCH] D143509: Move the BySpelling map to IncludeStructure.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/Headers.cpp:303 + llvm::SmallVector Includes; + auto It = MainFileIncludesBySpelling.find(Spelling); + if (It == MainFileIncludesBySpelling.end()) VitaNuo wrote: > kadircet wrote: > > nit: >

[PATCH] D143509: Move the BySpelling map to IncludeStructure.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/Headers.cpp:303 + llvm::SmallVector Includes; + auto It = MainFileIncludesBySpelling.find(Spelling); + if (It == MainFileIncludesBySpelling.end()) nit: ``` llvm::SmallVector Includes; for

[PATCH] D144456: [clangd] Publish diagnostics with stale preambles

kadircet updated this revision to Diff 499224. kadircet marked 7 inline comments as done. kadircet added a comment. - Notify preamble peer before building an AST, to concurrently build fresh preamble & AST. - Refactoring for tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D143496: [clangd] Add support for missing includes analysis.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/IncludeCleaner.cpp:359 +TransformedInc.Angled = WrittenRef.starts_with("<"); +auto FE = SM.getFileManager().getFile(Inc.Resolved); +if (!FE) unfortunately `getFile` returns an

[PATCH] D144456: [clangd] Publish diagnostics with stale preambles

kadircet created this revision. kadircet added a reviewer: sammccall. 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-extra.

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

kadircet added a comment. thanks for the review! Comment at: clang-tools-extra/clangd/Preamble.cpp:509 + // Check if AlternateLine matches all lines in the range. + if (llvm::any_of( + llvm::zip_equal(RangeContents, sammccall wrote: >

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

kadircet updated this revision to Diff 498351. kadircet marked 4 inline comments as done. kadircet added a comment. - Use direct comparison of ArrayRefs - optional for closest - loop over lookup results rather than through iterator Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

kadircet added inline comments. Comment at: clang-tools-extra/clangd/Preamble.cpp:625 +// same spelling. +static std::vector patchDiags(llvm::ArrayRef BaselineDiags, +const ScannedPreamble , sammccall wrote: > I think this

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

kadircet updated this revision to Diff 498308. kadircet marked 6 inline comments as done. kadircet added a comment. - Move patch translation logic to a separate class - Perform eager copies and use `bool translateX(X&)` type of APIs instead of returning optionals. - Perform multi line content

[PATCH] D143509: Move the BySpelling map to IncludeStructure.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/Headers.h:167 + // Spelling should include brackets or quotes, e.g. . + llvm::SmallVector + mainFileIncludesWithSpelling(llvm::StringRef Spelling) const { we're still returning just the

[PATCH] D143496: [clangd] Add support for missing includes analysis.

kadircet added a comment. Thanks! Details mostly looks good, but i think we had different ideas about how the final diagnostics should look like. Let's try to clear that up a bit. My suggested proposal is: - Having a main diagnostic for each symbol that doesn't have a satisfying include in

[PATCH] D143906: [include-cleaner] Better support ambiguous std symbols

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/lib/FindHeaders.cpp:85 +Hints isPublicHeader(const FileEntry *FE, const PragmaIncludes *PI) { + return

[PATCH] D142836: [clangd] Add symbol mappings for `std::experimental::filesystem`

kadircet accepted this revision. kadircet added a comment. This revision is now accepted and ready to land. thanks! Comment at: clang/lib/Tooling/Inclusions/Stdlib/StdTsSymbolMap.inc:6 +// whatever. +// clang-format off +SYMBOL(absolute, std::experimental::filesystem::, )

[PATCH] D143906: [include-cleaner] Better support ambiguous std symbols

kadircet added inline comments. Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:104 + } + if (FName == "remove") { +if (FD->getNumParams() == 1) nit: `else if` Comment at:

[PATCH] D142836: [clangd] Add symbol mappings for `std::experimental::filesystem`

kadircet added inline comments. Comment at: clang/lib/Tooling/Inclusions/Stdlib/StdTsSymbolMap.inc:6 +// whatever. +// clang-format off +SYMBOL(absolute, std::experimental::filesystem::, ) zyounan wrote: > kadircet wrote: > > can you strip clang-format pragmas

[PATCH] D143112: [clang] Support parsing comments without ASTContext

kadircet added a comment. Thanks, this looks like a good start. I left some comments about the details of implementation, as for placing and APIs in general, i think it's useful to see how things will be built on top inside clangd to make the right call here. Comment at:

[PATCH] D142836: [clangd] Add symbol mappings for `std::experimental::filesystem`

kadircet added a comment. thanks a lot! mostly LG couple more nits Comment at: clang/lib/Tooling/Inclusions/Stdlib/StdTsSymbolMap.inc:1 +// These symbols are exported from N4100[fs.filesystem.synopsis], the final +// draft for experimental filesystem. Note that not all of

[PATCH] D143640: [Tooling/Inclusion] Support overload symbols in the stdlib.

kadircet added a comment. In D143640#4122603 , @hokein wrote: > Thanks for the comment, putting more thoughts. > > In D143640#4121998 , @kadircet > wrote: > >> All this complexity for handling only 4 symbols

[PATCH] D143436: [clangd] Apply standard adaptors to CDBs pushed from LSP

kadircet added a comment. > There is a discrepancy between how clangd processes CDB loaded from JSON file > on disk and pushed via LSP. That's actually because we model compile commands pushed via LSP as a "fixed compilation database" rather than a json compilation database (you can check the

[PATCH] D143260: [clangd] Add semantic token for labels

kadircet added a comment. thanks for the patch, but could you elaborate a little bit on "why this is useful for clients". in theory semantic highlighting tries to provide annotations for tokens that are hard to disambiguate by a syntax highlighter due to need for context. at hindsight i can't

[PATCH] D142836: [clangd] Add symbol mappings for `std::experimental::filesystem`

kadircet added a comment. Thanks for updating this, as I mentioned in https://reviews.llvm.org/D143319#4115186, we should put these symbols into their own symbol map. ATM `StdSymbolMap.inc` is still generated by an automated tool and shouldn't be modified by hand. So can you rather put these

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

kadircet updated this revision to Diff 496907. kadircet edited the summary of this revision. kadircet added a comment. - Update commit message Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143096/new/ https://reviews.llvm.org/D143096 Files:

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

kadircet updated this revision to Diff 496905. kadircet added a comment. - Reflow comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143096/new/ https://reviews.llvm.org/D143096 Files: clang-tools-extra/clangd/ParsedAST.cpp

[PATCH] D143096: [clangd] Provide patched diagnostics with preamble patch

kadircet updated this revision to Diff 496900. kadircet marked 12 inline comments as done. kadircet added a comment. - Change patching logic to iterate over diags instead of preamble lines - Change translation logic to: - Preserve a diagnostic if its range can be translated. - Preserve all

[PATCH] D143597: [clangd] Drop includes from disabled PP regions in preamble patch

This revision was automatically updated to reflect the committed changes. Closed by commit rG19659b5f0dd1: [clangd] Drop includes from disabled PP regions in preamble patch (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D143197: [clangd] Fix bugs in main-file include patching for stale preambles

This revision was automatically updated to reflect the committed changes. Closed by commit rGfae01d175a29: [clangd] Fix bugs in main-file include patching for stale preambles (authored by kadircet). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D143640: [Tooling/Inclusion] Support overload symbols in the stdlib.

kadircet added a comment. All this complexity for handling only 4 symbols feels wrong. is this the whole set? are there any other symbols that fall under this class? can we disambiguate all of them solely based on number of parameters? Downsides: - We are relying heavily on number of

[PATCH] D143509: Move the BySpelling map to IncludeStructure.

kadircet added a comment. oh forgot to mention, could you also please add some tests into llvm-project/clang-tools-extra/clangd/unittests/HeadersTests.cpp ? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143509/new/

[PATCH] D143509: Move the BySpelling map to IncludeStructure.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/Headers.cpp:76 +Out->MainFileIncludesBySpelling.try_emplace(Inc.Written) +.first->second.push_back(static_cast(*Inc.HeaderID)); } right now we're only storing

[PATCH] D143638: [clangd] Move function body to out-of-line: unnamed class method incorrect moving

kadircet accepted this revision. kadircet added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/unittests/tweaks/DefineOutlineTests.cpp:89 + // Not available on methods of unnamed classes. + EXPECT_UNAVAILABLE(R"cpp( +

[PATCH] D143638: [clangd] Move function body to out-of-line: unnamed class method incorrect moving

kadircet added inline comments. Comment at: clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp:395 // Bail out in templated classes, as it is hard to spell the class name, i.e // if the template parameter is unnamed. could you move this

[PATCH] D143214: [include-mapping] Add C-compatibility symbol entries.

kadircet accepted this revision. kadircet added a comment. thanks, lgtm again Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D143214/new/ https://reviews.llvm.org/D143214 ___ cfe-commits mailing list

[PATCH] D143319: [clangd] Support iwyu pragma: no_include

kadircet added a comment. thanks a lot for the patch! we're migrating IWYU related functionality in clangd to make use of include-cleaner library nowadays. so can you actually move the IWYU no_include pragma handling logic into

[PATCH] D143214: [include-mapping] Add C-compatibility symbol entries.

kadircet added inline comments. Comment at: clang/lib/Tooling/Inclusions/Stdlib/StandardLibrary.cpp:218 + if (!D) // global scope. +return getMappingPerLang(L)->NamespaceSymbols->lookup(""); auto It = NamespaceCache.find(D); oh i actually missed the

[PATCH] D143496: [clangd] Add support for missing includes analysis.

kadircet added inline comments. Comment at: clang-tools-extra/clangd/Config.h:91 + enum class MissingIncludesPolicy { +/// Diagnose missing includes. rather than duplicating, what about renaming `UnusedIncludesPolicy` to `IncludesPolicy` and use it for

<    1   2   3   4   5   6   7   8   9   10   >