[PATCH] D140217: [lit] Script to automate use of %(line-n). Use in CodeComplete tests.

sammccall added a comment. In D140217#4004657 , @hokein wrote: > looks like you forgot to upload the new diff. The changes were trivial/mechanical so I wasn't planning on another round of review, but was waiting for the tests to finish before landing.

[PATCH] D140217: [lit] Script to automate use of %(line-n). Use in CodeComplete tests.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. sammccall marked 3 inline comments as done. Closed by commit rGcf9b25e0adc4: [lit] Script to automate use of %(line-n). Use in CodeComplete tests. (authored by

[PATCH] D140217: [lit] Script to automate use of %(line-n). Use in CodeComplete tests.

sammccall marked 6 inline comments as done. sammccall added inline comments. Comment at: llvm/utils/relative_lines.py:17 + +USAGE = """Example usage: +find clang/test/CodeCompletion | grep -v /Inputs/ | \ hokein wrote: > This variable is not used in

[PATCH] D140217: [lit] Script to automate use of %(line-n). Use in CodeComplete tests.

sammccall updated this revision to Diff 483547. sammccall edited the summary of this revision. sammccall added a comment. add bug link Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D140217/new/ https://reviews.llvm.org/D140217 Files:

[PATCH] D140217: [lit] Script to automate use of %(line-n). Use in CodeComplete tests.

sammccall created this revision. sammccall added a reviewer: hokein. Herald added a project: All. sammccall requested review of this revision. Herald added a reviewer: jdoerfert. Herald added subscribers: llvm-commits, cfe-commits, sstefan1. Herald added projects: clang, LLVM. Tests where the

[PATCH] D140095: [include-cleaner] Fix the member-expr-access usage for sugar type.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. LG, thanks! Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:45 +if (Base->isPointerType()) + Base = Base->getPointeeType(); +// Unwrap the

[PATCH] D140095: [include-cleaner] Fix the member-expr-access usage for sugar type.

sammccall added a comment. Mostly comment nits and was ready to approve, but I think I found a bug (getAs) Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:46 + Base = Base->getPointeeType(); +if (const auto *TT = Base->getAs()) + return

[PATCH] D140095: [include-cleaner] Fix the member-expr-access usage for sugar type.

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:70 QualType Type = E->getBase()->IgnoreImpCasts()->getType(); report(E->getMemberLoc(), resolveType(Type)); return true; hokein wrote: > sammccall

[PATCH] D140044: [CodeComplete] Complete members of dependent `auto` variables

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG33e3edde4495: [CodeComplete] Complete members of dependent `auto` variables (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES

[PATCH] D140095: [include-cleaner] Fix the member-expr-access usage for sugar type.

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/lib/WalkAST.cpp:50 + return UT->getFoundDecl(); +if (const auto *TST = Type->getAs()) + return resolveTemplateName(TST->getTemplateName()); Resolving a template type to

[PATCH] D140044: [CodeComplete] Complete members of dependent `auto` variables

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. When the initializer of an `auto` variable is dependent, clang doesn't give the

[PATCH] D140029: [CodeComplete] Provide designated-init completions in (const) reference context

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGbd672e2fc038: [CodeComplete] Provide designated-init completions in (const) reference context (authored by sammccall). Repository: rG LLVM Github

[PATCH] D140029: [CodeComplete] Provide designated-init completions in (const) reference context

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

[PATCH] D139921: [include-cleaner] Ranking of providers based on hints

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/lib/AnalysisInternal.h:84 +/// Represents properties of a symbol provider. +enum class Hint : uint8_t { along with `SymbolLocation`, I think mixing our fundamental types and

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

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks for the explanation! It makes sense. And given this motivation I don't think inverting to "builtin" is a good idea. The impact is a bit limited because it's not a standard

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

sammccall accepted this revision. sammccall added a comment. LG, thanks! Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:33 + for (auto : locateSymbol(S)) +// FIXME: findHeaders in theory returns the same result for all source +// locations in the same

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

sammccall added a comment. For my part, I still need to understand why we want the `builtin`/`UserModified` modifier. (The `operator` highlight kind seems obvious to me). From earlier comments: >> Can you give some background here or on the bug tracker about what kind of >> distinction

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

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:44 + + // Cache for decl to header mappings, as the same decl might be referenced in + // multiple locations and finding providers for each location is expensive.

[PATCH] D139107: [clangd] Allow to build Clangd without decision forest

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Looks great, thanks! Comment at: clang-tools-extra/clangd/CMakeLists.txt:21 +option(CLANGD_DECISION_FOREST "Enable decision forest model for ranking code completion

[PATCH] D139202: Link with missing libs to fix broken shared build

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks, and sorry for the breakage! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139202/new/ https://reviews.llvm.org/D139202

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG1a8dd7425873: [include-cleaner] clang-include-cleaner can print/apply edits (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES

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

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/lib/Analysis.cpp:77 + bool Satisfied = false; + for (const Header : Providers) { + if (H.kind() == Header::Physical && H.physical() == MainFile)

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

sammccall updated this revision to Diff 479575. sammccall added a comment. delete dead code, add comment Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139013/new/ https://reviews.llvm.org/D139013 Files:

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

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:65 +llvm::ArrayRef MacroRefs, +const Includes &, const PragmaIncludes *PI, +const

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

sammccall updated this revision to Diff 479574. sammccall marked an inline comment as done. sammccall added a comment. Herald added a project: clang. replace fixIncludes impl with clang-format, tweak tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D139107: [clangd] Allow to build Clangd without decision forest

sammccall added inline comments. Comment at: clang-tools-extra/clangd/CMakeLists.txt:54 + list(APPEND COMPLETIONMODEL_SOURCES ${CMAKE_CURRENT_BINARY_DIR}/CompletionModel.cpp) + list(APPEND COMPLETIONMODEL_SOURCES decision-forest/DecisionForest.cpp) +else()

[PATCH] D139088: [clangd] Log diagnostics if we failed to create a preamble.

This revision was automatically updated to reflect the committed changes. Closed by commit rG00b9cefacbdf: [clangd] Log diagnostics if we failed to create a preamble. (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139088/new/

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

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/include-cleaner/unittests/LocateSymbolTest.cpp:60 + return true; +Out = ND; +return false;

[PATCH] D139088: [clangd] Log diagnostics if we failed to create a preamble.

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] D138520: [clangd] Make decision forest model optional

sammccall added inline comments. Comment at: clang-tools-extra/clangd/CodeComplete.h:129 +#if defined(CLANGD_DECISION_FOREST) +# define DEFAULT_RANKING_MODEL DecisionForest mgorny wrote: > sammccall wrote: > > this approach feels a bit heavy on the

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

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

[PATCH] D139014: [include-cleaner] Move RecordedPP::RecordedIncludes -> Includes in Types.h. NFC

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGd3714c2b277a: [include-cleaner] Move RecordedPP::RecordedIncludes - Includes in Types.h. NFC (authored by sammccall). Changed prior to commit:

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG9961fa1653a2: [include-cleaner] Record whether includes are spelled with angle quotes (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

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

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/include-cleaner/lib/AnalysisInternal.h:29 #include "llvm/ADT/STLFunctionalExtras.h" +#include +#include unused?

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

sammccall added inline comments. Comment at: clang/test/Parser/recovery.c:105 + unknown_t a; // expected-error {{unknown type name 'unknown_t'}} + unknown_t *b; // expected-error {{unknown type name 'unknown_t'}} + unknown_t const *c; // expected-error {{unknown type name

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

sammccall updated this revision to Diff 478961. sammccall added a comment. add test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D139018/new/ https://reviews.llvm.org/D139018 Files:

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

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. This is needed to accurately remove headers with

[PATCH] D139014: [include-cleaner] Move RecordedPP::RecordedIncludes -> Includes in Types.h. NFC

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. Requiring everything that wants to match Includes to depend on Record

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

sammccall created this revision. sammccall added a reviewer: kadircet. 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. This adds command-line flags to

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

This revision was automatically updated to reflect the committed changes. Closed by commit rGa53f89522f46: [clangd] Heuristic to avoid desync if editors are confused about newline-at-eof (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D138908: [clangd] Highlight "miscellaneous" types as `type` as a fallback.

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] D138505: [clangd] Don't run slow clang-tidy checks by default

sammccall updated this revision to Diff 478274. sammccall retitled this revision from "[clangd] Don't run slow clang-tidy checks" to "[clangd] Don't run slow clang-tidy checks by default". sammccall edited the summary of this revision. sammccall added a comment. Add tests of isFast Make isFast a

[PATCH] D138780: [include-cleaner] Merge 2 parseIWYUPragma impls in libToolingInclusions

sammccall added a comment. In D138780#3953229 , @ABataev wrote: > Looks like it breaks the build: > > llvm-project/clang-tools-extra/clangd/index/CanonicalIncludes.cpp: In > member function ‘virtual bool >

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rGc7fc0abf510f: [clangd] Add script to maintain list of fast clang-tidy checks (authored by sammccall). Repository: rG LLVM Github Monorepo

[PATCH] D138520: [clangd] Make decision forest model optional

sammccall added a comment. Thanks for doing this! Comment at: clang-tools-extra/clangd/CMakeLists.txt:52 + list(APPEND COMPLETIONMODEL_SOURCES ${CMAKE_CURRENT_BINARY_DIR}/CompletionModel.cpp) + add_definitions(-DCLANGD_DECISION_FOREST=1) +endif() Rather

[PATCH] D138780: [include-cleaner] Merge 2 parseIWYUPragma impls in libToolingInclusions

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG99b5ec1fd1a7: [include-cleaner] Merge 2 parseIWYUPragma impls in libToolingInclusions (authored by sammccall). Repository: rG LLVM Github

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

sammccall added a comment. In D137205#3952864 , @carlosgalvezp wrote: >> by design clang-format only performs white-space changes > > clang-format does support east/west const enforcement with the > `QualifierAlignment` option. From experience, I

[PATCH] D138780: [include-cleaner] Merge 2 parseIWYUPragma impls in libToolingInclusions

sammccall updated this revision to Diff 478170. sammccall added a comment. tweak whitespace and */ behavior Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138780/new/ https://reviews.llvm.org/D138780 Files: clang-tools-extra/clangd/Headers.cpp

[PATCH] D138780: [include-cleaner] Merge 2 parseIWYUPragma impls in libToolingInclusions

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/lib/Record.cpp:146 // to share the code? static llvm::Optional parseIWYUPragma(const char *Text) { + // Skip the comment start, // or /*. hokein wrote: > I'd like to have some

[PATCH] D138780: [include-cleaner] Merge 2 parseIWYUPragma impls in libToolingInclusions

sammccall updated this revision to Diff 478163. sammccall added a comment. clean dead tests Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138780/new/ https://reviews.llvm.org/D138780 Files: clang-tools-extra/clangd/Headers.cpp

[PATCH] D138780: [include-cleaner] Merge 2 parseIWYUPragma impls in libToolingInclusions

sammccall updated this revision to Diff 478162. sammccall retitled this revision from "[include-cleaner] Minor fixes to parseIWYUPragma:" to "[include-cleaner] Merge 2 parseIWYUPragma impls in libToolingInclusions". sammccall edited the summary of this revision. sammccall added a comment. Herald

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

sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/lib/FindHeaders.cpp:58 +bool isSpelledInMainFile(SourceLocation Loc, const SourceManager ) { + return SM.isWrittenInMainFile(Loc.isMacroID() ? SM.getSpellingLoc(Loc) : Loc); +} this

[PATCH] D138780: [include-cleaner] Minor fixes to parseIWYUPragma:

sammccall created this revision. sammccall added a reviewer: hokein. Herald added a subscriber: 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. - remove assert that can fail for

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG2c1fa734598c: Reland [Lex] Fix suggested spelling of /usr/bin/../include/foo (authored by sammccall). Changed prior to commit:

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

sammccall marked an inline comment as done. sammccall added inline comments. Comment at: clang/unittests/Lex/HeaderSearchTest.cpp:155 + addSearchDir("/x/../y/"); + EXPECT_EQ(Search.suggestPathToFileForDiagnostics("/x/../y/z", +

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

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. Herald added a subscriber: cfe-commits. This reverts commit 1dc0a1e5d220b83c1074204bd3afd54f3bac4270

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

sammccall added inline comments. Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:76 ConstructorOrDestructor, + UserProvided, ckandeler wrote: > sammccall wrote: > > sammccall wrote: > > > nridge wrote: > > > > ckandeler wrote: > > > > >

[PATCH] D138676: [include-cleaner] HTMLReport shows headers that would be inserted

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG6a95e67323dd: [include-cleaner] HTMLReport shows headers that would be inserted (authored by sammccall). Repository: rG LLVM Github Monorepo

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

sammccall added a comment. Regarding AugmentsSyntaxTokens: - in general this seems like a nice thing to support, though I'm not aware of "big" `!AugmentsSyntaxTokens` clients so it's not urgent - I think the right design for that is to (conditionally) run the lexer and mark only tokens that

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

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG8bed59c7e7da: [Lex] Fix suggested spelling of /usr/bin/../include/foo (authored by sammccall). Changed prior to commit:

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

sammccall marked 2 inline comments as done. sammccall 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); kadircet wrote:

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

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. Herald added a subscriber: cfe-commits. Since D60873 we remove dotdots from the search

[PATCH] D138676: [include-cleaner] HTMLReport shows headers that would be inserted

sammccall updated this revision to Diff 477809. sammccall added a comment. fix bold/italic of header-to-insert on symbol refs Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138676/new/ https://reviews.llvm.org/D138676 Files:

[PATCH] D138676: [include-cleaner] HTMLReport shows headers that would be inserted

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

[PATCH] D60873: [clang][HeaderSuggestion] Handle the case of dotdot with an absolute path

sammccall added a comment. Herald added projects: clang-tools-extra, All. I think this introduced a bug: if your search path has `/usr/bin/../include/c++/v1`, then you'll end up with `FileEntry`s with `Name` like `/usr/bin/../include/c++/v1/__utility/move.h`. Calling the `FileEntry` overload

[PATCH] D138583: Create unused non-trivially-destructible check in clang-tidy

sammccall added a comment. In D138583#3949604 , @ankineri wrote: > I do agree that having it as part of `-Wunused-variable` is better on many > aspects, but I have a few concerns about it. > The proposed check can be applied to any type including STL

[PATCH] D138649: [include-cleaner] Show details for #include directives (used/unused)

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG3e658abd4147: [include-cleaner] Show details for #include directives (used/unused) (authored by sammccall). Repository: rG LLVM Github Monorepo

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

This revision was automatically updated to reflect the committed changes. Closed by commit rG0cb2dd5322f4: [include-cleaner] Make Symbol (and Macro) hashable. (authored by sammccall). Changed prior to commit: https://reviews.llvm.org/D138648?vs=477723=477768#toc Repository: rG LLVM Github

[PATCH] D138583: Create unused non-trivially-destructible check in clang-tidy

sammccall added a comment. Extending --warn-unused to other types seems preferable in a few ways: - clang warnings are more accessible than clang-tidy checks - avoids duplicating implementation - places description of the types along with the types, instead of in clang-tidy config - avoids

[PATCH] D138649: [include-cleaner] Show details for #include directives (used/unused)

sammccall added a comment. Demo https://htmlpreview.github.io/?https://gist.githubusercontent.com/sam-mccall/24f7bcf835052d4ddcf6dad4f26a500c/raw/db4791b0b80f5c67c6d25c9090bf8166e362c556/PathMapping.cpp.html Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D138649: [include-cleaner] Show details for #include directives (used/unused)

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

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

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. For now, we decided not to add operator< or handle other variants.

[PATCH] D138219: [include-cleaner] Show includes matched by refs in HTML report.

This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rG19ab2a671eb3: [include-cleaner] Show includes matched by refs in HTML report. (authored by sammccall). Changed prior to commit:

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

sammccall updated this revision to Diff 477443. sammccall added a comment. Script reads/writes from file rather than redirecting output. Update .inc file to show "no-op" changes. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138491/new/

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

sammccall planned changes to this revision. sammccall added inline comments. Comment at: clang-tools-extra/clangd/ConfigCompile.cpp:518 + +if (F.SlowChecks.has_value()) + Out.Apply.push_back([V = **F.SlowChecks](const Params &, Config ) { kadircet

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

sammccall added inline comments. Comment at: clang-tools-extra/clangd/ParsedAST.cpp:487 + } + CTFactories = std::move(FastFactories); +} njames93 wrote: > Not exactly related but surely both check factories could be made into static > variables

[PATCH] D138219: [include-cleaner] Show includes matched by refs in HTML report.

sammccall updated this revision to Diff 477429. sammccall retitled this revision from "[include-cleaner] Show includes matched by refs in HTML report. Demo:

[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 marked an inline comment as done. sammccall added inline comments. Comment at: clang-tools-extra/include-cleaner/lib/HTMLReport.cpp:116 FileID File; + const FileEntry *FE; hokein wrote: > nit: the `FE` can be removed -- we have the FileID, we can

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

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

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

sammccall added a comment. In D138546#3946046 , @cpsauer wrote: > Sam, my read, too, is that the memoizing design isn't safe--also that the key > bug is preexisting. > Nathan and I were thinking, though, that we'd should post this incremental > fix

[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#3946020 , @sammccall wrote: > In D138219#3945954 , @hokein wrote: > >> E.g. for the following case, the `UsingShadowDecl` refers to the *primary* >> template decl, which is

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

sammccall marked 3 inline comments as done. sammccall added inline comments. Comment at: clang-tools-extra/clangd/TidyFastChecks.py:41 +def read_old_fast(path): +text = subprocess.check_output(["cpp", "-P", "-FAST(C,T)=C", path]) +for line in text.splitlines():

[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. (I do think the outstanding issues are not related to this patch, so this is ready for review) In D138219#3945954 , @hokein wrote: > E.g. for the following case, the `UsingShadowDecl` refers to the *primary* > template decl,

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

sammccall added a comment. My biggest concern here is propagating more information here without including it in the cache key. Originally the computation was strictly using the cache key as input, but started using the command-line flags in D73453 . @kadircet

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

sammccall added a comment. Ideas on testing welcome. Does it make sense to rely on the fact that `misc-const-correctness` is always slow? :-D Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D138505/new/ https://reviews.llvm.org/D138505

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

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

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

sammccall created this revision. sammccall added a reviewer: kadircet. Herald added subscribers: abrachet, phosek, arphaman. Herald added a reviewer: njames93. Herald added a project: All. sammccall requested review of this revision. Herald added subscribers: cfe-commits, sstefan1, MaskRay,

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

This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rGfd733a65de5c: [clangd] Extend --check to time clang-tidy checks, so we can block slow ones (authored by sammccall). Changed prior to commit:

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

sammccall marked 2 inline comments as done. sammccall added inline comments. Comment at: clang-tools-extra/clangd/tool/Check.cpp:66 +llvm::cl::desc("Print the overhead of checks matching this glob"), +llvm::cl::init("")); + kadircet wrote: > i guess we

[PATCH] D138425: [clangd] Parameter hints for template specialization

sammccall added a comment. In D138425#3941183 , @v1nh1shungry wrote: > @sammccall Thank you for reviewing and giving suggestions! > > I must admit I didn't use it for very long. But I do think this is helpful, > at least for templates I'm unfamiliar

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

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. LG from a thread-safety perspective, thanks! Approving because it looks mechanically "obviously correct", but you may want to wait for another reviewer to confirm the API changes are

[PATCH] D138425: [clangd] Parameter hints for template specialization

sammccall added a comment. The code looks pretty good and this makes a lot of logical sense. However, there's an ugly practical issue: it's overwhelmingly common for template parameters to have cryptic and useless names. It seems like turning `vector` into `vector` actually makes things worse.

[PATCH] D137794: [clangd] Enable configuring include insertions

sammccall added a comment. The replacement rules design is just too complicated I'm afraid. It's unclear that it solves a widespread problem, and even when there is one and the solution applies, the result is still difficult to use. Angles vs quotes is something we've had multiple reports on,

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

sammccall added a comment. In D138287#3940602 , @kito-cheng wrote: > Apparently I missed those comments during llvm dev meeting, I'll figure out a > fix soon. Thanks a lot! Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D137894: [clangd] Add extension for adding context (enclosing function or class) in references results

sammccall added a comment. Since this is a protocol extension, you might want to add an end-to-end test: variant of `clang-tools-extra/clangd/test/xrefs.test` (`xrefs-container.test`? to avoid complicating all the existing tests). And one this lands, one of us should update

[PATCH] D137943: [clangd] Mark "override" and "final" as keywords

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. LGTM with optional mechanical replacement of `Keyword` with `Modifier`. Comment at: clang-tools-extra/clangd/SemanticHighlighting.h:52 Macro, + Keyword,

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

sammccall added a subscriber: kito.cheng. sammccall added a comment. I think there almost certainly *is* a better way here, but it may be a significant amount of work and we need to get back to a correct state (revert doesn't seem feasible). Not approving yet in the hope that @kito-cheng

[PATCH] D137894: [clangd] Add extension for adding context (enclosing function or class) in references results

sammccall added inline comments. Comment at: clang-tools-extra/clangd/Protocol.h:231 +/// Special Location-like type to be used for textDocument/references +/// Contains an additional field for the context in which the reference occurs This doesn't seem as

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

sammccall added a comment. In D137205#3939720 , @njames93 wrote: > @sammccall I have a feeling you're gonna want to examine this checks > feasibility in clangd. Thanks! As it uses the CFG, by default we're going to have to turn it off (AFAIK building

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

sammccall added a comment. Not sure this is ready for review again, ignore me if not... I'm still not sure why this is correct in principle. Without that, if someone finds a misparse 6 months from now I don't know how we determine where to fix it. For example, this path is called from

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