[PATCH] D136594: [clangd] Add support for semantic token type "operator"

sammccall added a comment. In D136594#3940143 , @nridge wrote: > I believe highlighting of **built-in** operators should be out of scope for > semantic highlighting, at least in the default mode; client-side highlighting > should be sufficient for

[PATCH] D138219: [include-cleaner] Show includes matched by refs in HTML report. Demo: https://htmlpreview.github.io/?https://gist.githubusercontent.com/sam-mccall/ecee6869e37af3db28089b64d8dce806/ra

sammccall added a comment. In D138219#3936279 , @hokein wrote: > Thanks, I like the idea of marking missing-include refs. > > I haven't read the code yet, and below are all my findings when I played with > the demo html (some of them are unrelated to

[PATCH] D137909: [Support] Allow complex names for annotation points and ranges via ${}

sammccall added a comment. (please update the commit message too since the design changed) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137909/new/ https://reviews.llvm.org/D137909 ___ cfe-commits

[PATCH] D137909: [Support] Allow complex names for annotation points and ranges via ${}

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Fantastic, thanks! Comment at: clang-tools-extra/clangd/unittests/Annotations.h:29 - Position point(llvm::StringRef Name = "") const; + clangd::Position

[PATCH] D138219: [include-cleaner] Show includes matched by refs in HTML report. Demo: https://htmlpreview.github.io/?https://gist.githubusercontent.com/sam-mccall/ecee6869e37af3db28089b64d8dce806/ra

sammccall created this revision. sammccall added a reviewer: hokein. Herald added a subscriber: mgrang. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github

[PATCH] D137677: [include-cleaner] add macro symbols and implicit refs to HTML report

This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rG267577325876: [include-cleaner] add macro symbols and implicit refs to HTML report (authored by sammccall). Changed prior to commit:

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

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/lib/AnalysisInternal.h:87 +/// FIXME: Expose in public API for decision making (ranking, ignoring, etc.). +enum class Hint : uint8_t { + /// Declaration for the symbol, which is provided by all

[PATCH] D137909: [Support] Allow complex names for annotation points and ranges via ${}

sammccall added a comment. Thanks! sorry about continuing to drift this patch, but this looks like a great improvement. Critical comments below are: - dropping `Optional` and just use StringRef - using one consistent way to to include payloads across the API. I think `pair` is actually best

[PATCH] D138193: [Serialization] Fix crash writing non-module PCH in non-module mode including modular headers

sammccall created this revision. sammccall added a reviewer: kadircet. Herald added a project: All. sammccall requested review of this revision. Herald added subscribers: cfe-commits, ilya-biryukov. Herald added a project: clang. Ran into this when trying to turn off modules mode in clangd. Took

[PATCH] D137962: [clang][Tooling] Make the filename behaviour consistent

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Code change LGTM, i think the doc change should be dropped or reshaped though. Comment at: clang/docs/JSONCompilationDatabase.rst:89 same file, for example if the

[PATCH] D137677: [include-cleaner] add macro symbols and implicit refs to HTML report

sammccall added a comment. Updated the demo link, PTAL (significant changes since last review) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137677/new/ https://reviews.llvm.org/D137677 ___ cfe-commits

[PATCH] D137677: [include-cleaner] add macro symbols and implicit refs to HTML report

sammccall updated this revision to Diff 475068. sammccall added a comment. format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137677/new/ https://reviews.llvm.org/D137677 Files:

[PATCH] D137677: [include-cleaner] add macro symbols and implicit refs to HTML report

sammccall updated this revision to Diff 474738. sammccall added a comment. - hover card follows the internal data model: symbol -> location -> header (->include, later) - don't use public API, so we can get at internal details Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D137825: [clang-include-cleaner] make SymbolLocation a real class, move FindHeaders

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG7943169273b2: [clang-include-cleaner] make SymbolLocation a real class, move FindHeaders (authored by sammccall). Repository: rG LLVM Github

[PATCH] D137697: Move the isSelfContained function from clangd to libtooling.

sammccall added inline comments. Comment at: clang/lib/Tooling/Inclusions/Header.cpp:64 + // particular preprocessor state, usually set up by another header. + return !isDontIncludeMeHeader(SM.getBufferData(SM.translateFile(FE))); +} kadircet wrote: >

[PATCH] D137825: [clang-include-cleaner] make SymbolLocation a real class, move FindHeaders

sammccall added a comment. In D137825#3921273 , @tschuett wrote: > You could: > > static_assert(std::variant_size_v<> == 2); > > Kind kind() const { if (std::holds_alternative(Storage)) > return Kind::Physical; return Kind::Standard; } > >

[PATCH] D137697: Move the isSelfContained function from clangd to libtooling.

sammccall added inline comments. Comment at: clang/lib/Tooling/Inclusions/Header.cpp:64 + // particular preprocessor state, usually set up by another header. + return !isDontIncludeMeHeader(SM.getBufferData(SM.translateFile(FE))); +} kadircet wrote: >

[PATCH] D137825: [clang-include-cleaner] make SymbolLocation a real class, move FindHeaders

sammccall created this revision. sammccall added a reviewer: kadircet. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. - replace SymbolLocation std::variant with enum-exposing version

[PATCH] D137320: [include-cleaner] Initial version for the "Location=>Header" step

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/unittests/AnalysisTest.cpp:33 TEST(WalkUsed, Basic) { // FIXME: Have a fixture for setting up tests. llvm::Annotations Code(R"cpp( sammccall wrote: > @kadircet gentle reminder

[PATCH] D137320: [include-cleaner] Initial version for the "Location=>Header" step

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:63 +// FIXME: use a real Class! +using SymbolLocation = std::variant; + hokein wrote: > kadircet wrote: > > let's move this to

[PATCH] D137693: [NFC] [C++20] [Modules] [clangd] Add test for code completion for C++20 Named Modules

sammccall added a comment. I think at this point we specifically don't want to commit to preserving particular modules behavior, this is more or less what the unsupported state means (https://github.com/clangd/clangd/issues/1293). In particular the pattern where modules are built externally by

[PATCH] D137770: [docs] Introduce clangd part in StandardCPlusPlusModules

sammccall added a comment. The status quo for modules on clangd is all flavors are completely unsupported: https://github.com/clangd/clangd/issues/1293. I don't think we should encourage people to set up workflows that are known not to work properly and will probably break in future. It's true

[PATCH] D137677: [include-cleaner] add macro symbols and implicit refs to HTML report

sammccall created this revision. sammccall added a reviewer: hokein. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Demo: https://gistpreview.github.io/?fec9b77c726cfb3cc7c424b197e3f68c

[PATCH] D133757: [clangd] Perform system include extraction inside CommandMangler

sammccall added a comment. In D133757#3916028 , @nridge wrote: > In D133757#3915995 , @ayzhao wrote: > >> This change is causing system-include-extractor.test >>

[PATCH] D137674: [clangd] Make system-include-extractor.test more resilient in the face of paths with special characters

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. LG, I committed https://github.com/llvm/llvm-project/commit/96d771c4707df9b3153ea5a8451f2bbbe17c8917 but was too lazy to add the comment, feel free to land this or not. Repository:

[PATCH] D136723: [include-cleaner] Record main-file macro occurences and includes

sammccall added a comment. (and sorry, it seems I was sitting on a pile of comments I thought I'd sent, LMK if I should follow up on them) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136723/new/ https://reviews.llvm.org/D136723

[PATCH] D136723: [include-cleaner] Record main-file macro occurences and includes

sammccall added a comment. In D136723#3915853 , @maryammo wrote: > This commit causes build failure on > `https://lab.llvm.org/buildbot/#/builders/121/builds/24947` : > I was able to reproduce the failure and by reverting this commit locally it >

[PATCH] D137644: [include-cleaner] pass through recorded macro refs in walkUsed

This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rG38cccb906603: [include-cleaner] pass through recorded macro refs in walkUsed (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST

[PATCH] D137644: [include-cleaner] pass through recorded macro refs in walkUsed

sammccall updated this revision to Diff 474008. sammccall added a comment. rebase Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D137644/new/ https://reviews.llvm.org/D137644 Files:

[PATCH] D137644: [include-cleaner] pass through recorded macro refs in walkUsed

sammccall created this revision. sammccall added a reviewer: kadircet. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo

[PATCH] D136723: [include-cleaner] Record main-file macro occurences and includes

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG8249dc21046a: [include-cleaner] Record main-file macro occurences and includes (authored by sammccall). Changed prior to commit:

[PATCH] D136710: [include-cleaner] Add the missing parts of Symbol and Header clases.

This revision was automatically updated to reflect the committed changes. sammccall marked 2 inline comments as done. Closed by commit rG5426a1e51d20: [include-cleaner] Add the missing parts of Symbol and Header clases. (authored by sammccall). Changed prior to commit:

[PATCH] D133757: [clangd] Perform system include extraction inside CommandMangler

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

[PATCH] D137020: [clang][AST] Handle variable declaration with unknown typedef in C

sammccall added a comment. I admire the goals of this patch, but I think it needs some explanation "why we expect this to be roughly correct/safe", rather than just trying to think of cases that might go wrong and patch them. The test coverage for incorrect code is not good, I'd expect to need

[PATCH] D136283: [clang-tools-extra] [clangd] Split huge generated CompletionModel.cpp into smaller files

sammccall added a comment. I don't think we should add significant build-system complexity here in order to support the completion model on ppc32. Do people actually use clangd on ppc32 machines? (The linked bug calls this a clang build failure, but this code is not part of clang). If we need

[PATCH] D137252: [include-cleaner] Testing helpers for ast walking

sammccall added a comment. Agree there are readability concerns here (long macro names, out-of-line kinds, complicated macros). My suggestion would be to treat RefKind as an optional feature. Most tests would just assert the set of marked locations and test the cases where kind is interesting

[PATCH] D136071: [include-cleaner] Add PragmaIncludes which handles include-mapping pragmas.

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:41 +// spelling header rather than the header directly defines the symbol. +class PragmaIncludes { +public: hokein wrote: > kadircet wrote: > >

[PATCH] D136782: [include-cleaner] Move the writeHTMLReport to the public header, NFC.

sammccall added a comment. OOC, Is there something prompting doing this immediately? I *did* originally plan to mechanically move this and we *can* but... The API isn't even approximately the right shape (FileID should go away as we've baked main-file assumption in, Roots needs to be paired

[PATCH] D136723: [include-cleaner] Record main-file macro occurences and includes

sammccall updated this revision to Diff 471155. sammccall added a comment. const Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136723/new/ https://reviews.llvm.org/D136723 Files:

[PATCH] D136710: [include-cleaner] Add the missing parts of Symbol and Header clases.

sammccall marked an inline comment as done. sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Types.h:49 + Kind kind() const { return static_cast(Storage.index()); } + bool operator==(const Symbol ) const { return

[PATCH] D136723: [include-cleaner] Record main-file macro occurences and includes

sammccall updated this revision to Diff 471150. sammccall marked 10 inline comments as done. sammccall added a comment. address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136723/new/ https://reviews.llvm.org/D136723 Files:

[PATCH] D136710: [include-cleaner] Add the missing parts of Symbol and Header clases.

sammccall updated this revision to Diff 471143. sammccall added a comment. Add comment on variant/enum order match. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136710/new/ https://reviews.llvm.org/D136710 Files:

[PATCH] D136071: [include-cleaner] WIP: Add PragmaIncludes which handles include-mapping pragmas.

sammccall added a comment. LG from my side Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:58 + // Line numbers for the include directives in the main file that have the + // `IWYU pragma: keep` right after. + llvm::DenseSet ShouldKeep;

[PATCH] D136723: [include-cleaner] Record main-file macro occurences and includes

sammccall updated this revision to Diff 470818. sammccall marked 5 inline comments as done. sammccall added a comment. address comments Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136723/new/ https://reviews.llvm.org/D136723 Files:

[PATCH] D136725: [Lex] Stop allocating/deallocating MacroInfo on a linked list. NFC

This revision was automatically updated to reflect the committed changes. Closed by commit rGe04482456713: [Lex] Stop allocating/deallocating MacroInfo on a linked list. NFC (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D136725: [Lex] Stop allocating/deallocating MacroInfo on a linked list. NFC

sammccall added a comment. (I'm sure this doesn't matter much, I just ran into it while trying to understand lifetimes of various macro description objects) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136725/new/

[PATCH] D136725: [Lex] Stop allocating/deallocating MacroInfo on a linked list. NFC

sammccall created this revision. sammccall added a reviewer: hokein. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This list was originally used for to make sure MacroInfo's clever memory management

[PATCH] D136723: [include-cleaner] Record main-file macro occurences and includes

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Record.h:75 +// - for a logical file like , we check Spelled +llvm::SmallVector match(Header H) const; + in the prototype I reimplemented this

[PATCH] D136723: [include-cleaner] Record main-file macro occurences and includes

sammccall created this revision. sammccall added reviewers: kadircet, hokein. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. The occurrences are roots for finding used headers, like

[PATCH] D136293: [IncludeCleaner] Add public API

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:27 + Symbol(Decl ) : Storage() {} + Symbol(tooling::stdlib::Symbol S) : Storage(S) {} + Doh, I was skimming and missed that you removed the

[PATCH] D136539: [Lex] Bring back the magic number 50 in updateConsecutiveMacroArgTokens.

sammccall accepted this revision. sammccall added a comment. LG other than removing the redundant same-sloc-address check again (sorry!) Comment at: clang/lib/Lex/TokenLexer.cpp:1010 +unsigned distance = +T.getLocation().getRawEncoding() -

[PATCH] D136710: [include-cleaner] Add the missing parts of Symbol and Header clases.

sammccall created this revision. sammccall added a reviewer: kadircet. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. Repository: rG LLVM Github Monorepo

[PATCH] D135859: [Includecleaner] Introduce RefType to ast walking

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:24 +/// Indicates the relation between the reference and the target. +enum class RefType { as discussed I moved the other vocab types into

[PATCH] D136539: [Lex] Bring back the magic number 50 in updateConsecutiveMacroArgTokens.

sammccall added a comment. In D136539#3882316 , @hokein wrote: >> This is a subtle change that needs careful review, and honestly should have >> a test. > > I agree, and thanks for the nice review comments! > > I'd like to add a unittest, but I don't

[PATCH] D133756: [clangd] Introduce CompileCommandsAdjuster

sammccall accepted this revision. sammccall added a comment. Still LG! Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.h:165 +// process a file (possibly different from the one in the command). +class CompileCommandsAdjuster { +public: nridge

[PATCH] D136567: [clangd] Avoid hanging in Selection when PP corrects the token sequence.

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

[PATCH] D136539: [Lex] Bring back the magic number 50 in updateConsecutiveMacroArgTokens.

sammccall added a comment. > This is a subtle change that needs careful review, and honestly should have a > test. Um, sorry for the grumpy tone. I thought this had already been quickly landed! Apologies... Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D136539: [Lex] Bring back the magic number 50 in updateConsecutiveMacroArgTokens.

sammccall added a comment. This is a subtle change that needs careful review, and honestly should have a test. I realize there's a breakage that needs to be fixed with some urgency and you believe you're just restoring the old behavior, however in that case the right course of action is to

[PATCH] D134137: [clangd] Return earlier when snippet is empty

sammccall added a comment. It's not surprising that we often don't crash here, the "obvious" lowering of `S.front()` is `*S.data()` which is perfectly valid (will be 0 for an empty string). One way to crash on this is to build with libstdc++'s debug iterators, which assert on this. Fedora

[PATCH] D136293: [IncludeCleaner] Add public API

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks! Looks good, feel free to address/disregard design comments as you see fit. Comment at:

[PATCH] D136212: [clangd] consider ~^foo() to target the destructor, not the type

This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rG62116c8f0b5b: [clangd] consider ~^foo() to target the destructor, not the type (authored by sammccall). Changed prior to commit:

[PATCH] D136212: [clangd] consider ~^foo() to target the destructor, not the type

sammccall marked 3 inline comments as done. sammccall added a comment. Ugh, sorry for the sloppiness, think I was tired. Comment at: clang-tools-extra/clangd/unittests/SelectionTests.cpp:469 {"struct foo { [[^~foo()]]; };", "CXXDestructorDecl"}, - // FIXME: The

[PATCH] D136248: [Index] consider `delete X` a reference to ~X() if it can be resolved

sammccall created this revision. sammccall added a reviewer: hokein. Herald added subscribers: kadircet, arphaman. Herald added a project: All. sammccall requested review of this revision. Herald added subscribers: cfe-commits, ilya-biryukov. Herald added projects: clang, clang-tools-extra. Fixes

[PATCH] D136229: [include-cleaner] Fix link errors when -DBUILD_SHARED_LIBS=ON

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

[PATCH] D136216: [clangd] go-to-def on new/delete targets the constructor

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

[PATCH] D136212: [clangd] consider ~^foo() to target the destructor, not the type

sammccall updated this revision to Diff 468741. sammccall added a comment. format Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136212/new/ https://reviews.llvm.org/D136212 Files: clang-tools-extra/clangd/Selection.cpp

[PATCH] D136212: [clangd] consider ~^foo() to target the destructor, not the type

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

[PATCH] D135956: [include-cleaner] Add include-cleaner tool, with initial HTML report

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. sammccall marked 4 inline comments as done. Closed by commit rG6fa0e026c87e: [include-cleaner] Add include-cleaner tool, with initial HTML report (authored by

[PATCH] D136071: [include-cleaner] WIP: Add PragmaIncludes which handles include-mapping pragmas.

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Hooks.h:38 +// spelling header rather than the header directly defines the symbol. +class PragmaIncludes { +public: sammccall wrote: > IWYU pragmas

[PATCH] D135956: [include-cleaner] Add include-cleaner tool, with initial HTML report

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Hooks.h:9 +// +// Where Analysis.h analyzes AST nodes and recorded preprocessor events, this +// file defines ways to capture AST and preprocessor information from a

[PATCH] D135956: [include-cleaner] Add include-cleaner tool, with initial HTML report

sammccall updated this revision to Diff 468343. sammccall marked 2 inline comments as done. sammccall added a comment. Herald added a project: clang. address comments, add tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135956/new/

[PATCH] D136071: [include-cleaner] WIP: Add PragmaIncludes which handles include-mapping pragmas.

sammccall added a comment. (It's a bit hard to evaluate this without the other pieces, but I suppose we need to start somewhere) Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Hooks.h:38 +// spelling header rather than the header directly defines

[PATCH] D136082: [clangd] Extend --check to time clang-tidy checks, so we can block slow ones

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

[PATCH] D127403: [clangd] Implement semantic token modifier "definition"

sammccall added a comment. In D127403#3861619 , @ckandeler wrote: > Everything good now? Yes :-) (I also marked it as accepted above, so feel free to land!) Thank you! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D136033: [clangd] Add an option to specify a workspece-level config file

sammccall added a comment. I have some concerns about adding an undefined "workspace" concept, and in general extending the config system in a way that's hard to use (because of the need to customize flags per workspace). Maybe best to discuss on the bug?

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

sammccall added a comment. In D54943#3854279 , @sammccall wrote: > FYI I've sent D135829 to block this check > from running in clangd, after tracking it down as the cause of a >10x > regression in reparse times in a

[PATCH] D135956: [include-cleaner] Add include-cleaner tool, with initial HTML report

sammccall added reviewers: kadircet, hokein. sammccall added a comment. Demo: https://htmlpreview.github.io/?https://gist.githubusercontent.com/sam-mccall/0cc7dd1f6c5605837966cb113babe591/raw/7c9a29142eca1fc1f486540f255586a0f72a9c9e/AST.html This is missing tests, my plan is: - add some basic

[PATCH] D135956: [include-cleaner] Add include-cleaner tool, with initial HTML report

sammccall created this revision. Herald added a subscriber: mgrang. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang-tools-extra. Herald added a subscriber: cfe-commits. The immediate goal is to start producing an HTML report to debug and

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

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/lib/AnalysisInternal.h:64 +using SymbolLocation = std::variant; +/// A set of locations that provides the declaration, while indicating if This is an important public API concept

[PATCH] D135859: [Includecleaner] Introduce RefType to ast walking

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:41 - void report(SourceLocation Loc, NamedDecl *ND) { + void report(SourceLocation Loc, NamedDecl *ND, RefType RT) { if (!ND || Loc.isInvalid()) using default

[PATCH] D135696: [pseudo] Document disambiguation design progress

This revision was automatically updated to reflect the committed changes. Closed by commit rGb13f7f9c0660: [pseudo] Document disambiguation design progress (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135696/new/

[PATCH] D135829: [clangd] Block clang-tidy misc-const-correctness check

This revision was automatically updated to reflect the committed changes. Closed by commit rGe78165f0ba1e: [clangd] Block clang-tidy misc-const-correctness check (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135829/new/

[PATCH] D54943: [clang-tidy] implement new check 'misc-const-correctness' to add 'const' to unmodified variables

sammccall added a comment. FYI I've sent D135829 to block this check from running in clangd, after tracking it down as the cause of a >10x regression in reparse times in a project that inadvertently enabled it (`misc-*`). Looking at the implementation, this

[PATCH] D135829: [clangd] Block clang-tidy misc-const-correctness check

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

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

sammccall accepted this revision. sammccall added a comment. Thanks, LG! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135712/new/ https://reviews.llvm.org/D135712 ___ cfe-commits mailing list

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

sammccall added inline comments. Comment at: cmake/Modules/FindGRPC.cmake:111 # They may be relative to the source dir or absolute (for generated protos). -function(generate_protos LibraryName ProtoFile) +macro(generate_protos_source GeneratedSource ProtoFile)

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. thanks! Comment at: cmake/Modules/FindGRPC.cmake:111 # They may be relative to the source dir or absolute (for generated protos). -function(generate_protos

[PATCH] D134303: [AST] Preserve more structure in UsingEnumDecl node.

sammccall added inline comments. Comment at: clang/include/clang/AST/DeclCXX.h:3618 + /// The source location of the 'enum' keyword. + SourceLocation EnumLoc; + /// 'qual::SomeEnum' as an EnumType, possibly with Elaborated/Typedef sugar. hokein wrote: > nit:

[PATCH] D134303: [AST] Preserve more structure in UsingEnumDecl node.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. sammccall marked 5 inline comments as done. Closed by commit rG2eaf6f973cac: [AST] Preserve more structure in UsingEnumDecl node. (authored by sammccall). Changed

[PATCH] D135712: [CMake] Fix FindGRPC cmake module to allow different layering

sammccall added a comment. Hmm, as mentioned on the bug, I think wrapping the deps and generated sources into a library is an important part of what the build rule does. If we need to split it up like this, I think a `generate_clang_protos` wrapper in FindGRPC.cmake might be the right place to

[PATCH] D135696: [pseudo] Document disambiguation design progress

sammccall added a comment. The diagrams are rendered by github's markdown view. My favorite offline tool to preview them is the "Markdown Preview Mermaid Support" VSCode extension. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135696/new/

[PATCH] D135696: [pseudo] Document disambiguation design progress

sammccall created this revision. sammccall added a reviewer: hokein. Herald added a project: All. sammccall requested review of this revision. Herald added subscribers: cfe-commits, alextsao1999. Herald added a project: clang-tools-extra. Need to take a break from this, so write down where we got

[PATCH] D135555: [libclang] CIndex: Visit UsingTypeLoc as TypeRef

This revision was automatically updated to reflect the committed changes. Closed by commit rG5d859a1cdee3: [libclang] CIndex: Visit UsingTypeLoc as TypeRef (authored by kiloalphaindia, committed by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D127403: [clangd] Implement semantic token modifier "definition"

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Just realized ParmVarDecls (and template args) are maybe more complicated than I thought: I suppose they're only really definitions if the function/template has a body (i.e. is itself a

[PATCH] D135555: [libclang] CIndex: Visit UsingTypeLoc as TypeRef

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/unittests/libclang/TestUtils.h:85 } + static std::string from_CXString(CXString cx_string) { +std::string

[PATCH] D127403: [clangd] Implement semantic token modifier "definition"

sammccall accepted this revision. sammccall added a comment. Thanks! Sorry for letting this drop. Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:79 + if (auto *Var = dyn_cast(Decl)) +return !isa(Var) && Var->isThisDeclarationADefinition(); + return

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

sammccall added a comment. In D135508#3846231 , @kadircet wrote: > i also would rather not have the workaround solely for an editor (we usually > try to address these on editor/plugin side). Yeah, for sure. My main concern is that in practice it won't

[PATCH] D135489: [clangd] Fix rename for symbol introduced by UsingDecl

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks! Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:169 +// For renaming, we're only interested in foo's declaration, so drop the other one +void

[PATCH] D135506: [clangd] FindTarget: UsingEnumDecl is not an alias

This revision was automatically updated to reflect the committed changes. Closed by commit rG2bb34cc462ff: [clangd] FindTarget: UsingEnumDecl is not an alias (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135506/new/

[PATCH] D135506: [clangd] FindTarget: UsingEnumDecl is not an alias

sammccall added a comment. In D135506#3844919 , @tom-anders wrote: > lgtm, thanks! With this change in place, we could probably ajdust > `pickDeclToUse` in Hover.cpp (introduced in https://reviews.llvm.org/D133664) > to also check for `UsingDecl`

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