[PATCH] D133968: [clangd] Enable standard library index by default.

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG79ed24eea37f: [clangd] Enable standard library index by default. (authored by sammccall). Repository: rG LLVM Github

[PATCH] D133968: [clangd] Enable standard library index by default.

sammccall added a comment. In D133968#3794850 , @kadircet wrote: > thanks, LG but seems to be failing on windows :/ This seemed very plausible but then none of the actual tests hit this codepath, and everything passes on rerun :-\ Repository: rG

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

sammccall added a comment. (also the desyncs are incredibly annoying, and happen pretty much any time you clear all text out of a file) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135508/new/ https://reviews.llvm.org/D135508

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

sammccall added a comment. Sorry to dump more patches on you, but I want your opinion on whether we should carry such a hack. My initial inclination is no, but the class of bug is not very hard to understand, could plausibly come up in other clients, and the fix is fairly simple. I'd like to

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

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

[PATCH] D133968: [clangd] Enable standard library index by default.

sammccall updated this revision to Diff 466243. sammccall added a comment. tickle premerge test Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133968/new/ https://reviews.llvm.org/D133968 Files: clang-tools-extra/clangd/Config.h

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

sammccall added inline comments. 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 filterBaseUsingDecl(llvm::DenseSet& Decls) { + if (Decls.size() == 2) {

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

sammccall created this revision. sammccall added a reviewer: tom-anders. Herald added subscribers: jeroen.dobbelaere, 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:

[PATCH] D135440: [SourceManager] Speedup getFileIDLocal with a separate Offset Table.

sammccall added a comment. It'd be great to have some numbers for overall memory increase based on clang rather than clangd, that's a more critical case and has a different memory pattern. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135440/new/

[PATCH] D135440: [SourceManager] Speedup getFileIDLocal with a separate Offset Table.

sammccall added a reviewer: aaron.ballman. sammccall added a subscriber: aaron.ballman. sammccall added a comment. This looks good to me, I think we should ask @aaron.ballman about the memory increase. If we wanted to eliminate SLocEntry::Offset then we could add

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

sammccall added a reviewer: hokein. sammccall added a comment. ping, +additional potential reviewer Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134303/new/ https://reviews.llvm.org/D134303 ___

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

sammccall added a comment. (Thanks for fixing this BTW!) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135489/new/ https://reviews.llvm.org/D135489 ___ cfe-commits mailing list

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

sammccall added a comment. Want to probe a bit at the behavior we're aiming for here. TL;DR is I think the handling is basically right but considering the general case tells us how to simplify filterBaseUsingDecl. --- The testcase is great to illustrate the common case. The thing we need to

[PATCH] D135356: [Format] Fix crash when hitting eof while lexing JS template string

sammccall added a comment. Fixed by 4b53c00173163774d32125fbcae283a46a9a4b19 I think. (@kadircet suggested a possible second crash, I couldn't get it to crash so included the testcase with this patch. Turns out it does

[PATCH] D135226: [clangd] Optimize Dex::generateProximityURIs().

This revision was automatically updated to reflect the committed changes. Closed by commit rG561443818a15: [clangd] Optimize Dex::generateProximityURIs(). (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135226/new/

[PATCH] D135356: [Format] Fix crash when hitting eof while lexing JS template string

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG882a05afa17f: [Format] Fix crash when hitting eof while lexing JS template string (authored by sammccall). Changed prior to commit:

[PATCH] D135356: [Format] Fix crash when hitting eof while lexing JS template string

sammccall marked an inline comment as done. sammccall added inline comments. Comment at: clang/lib/Format/FormatTokenLexer.cpp:767 if (Offset[0] == '\\') { ++Offset; // Skip the escaped character. } else if (Offset + 1 < Lex->getBuffer().end() && Offset[0] ==

[PATCH] D135314: [clangd] Avoid scanning up to end of file on each comment!

sammccall added a comment. BTW it looks like the 8% profile I saw was an outlier, 3-4% seems more typical. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135314/new/ https://reviews.llvm.org/D135314 ___

[PATCH] D133647: [clang-format] Parse the else part of `#if 0`

sammccall added a comment. This seems to have regressed comment alignment on unrelated directives: #if 0 #endif #if X int something_fairly_long; // Align here please #endif // Should be aligned These comments were aligned before this patch, but are no longer.

[PATCH] D135356: [Format] Fix crash when hitting eof while lexing JS template string

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. Different loop termination conditions resulted in confusion of whether *Offset

[PATCH] D135314: [clangd] Avoid scanning up to end of file on each comment!

This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rG5d2d527c32da: [clangd] Avoid scanning up to end of file on each comment! (authored by sammccall). Changed prior to commit:

[PATCH] D135314: [clangd] Avoid scanning up to end of file on each comment!

sammccall marked 3 inline comments as done. sammccall added inline comments. Comment at: clang-tools-extra/clangd/Headers.cpp:25 -const char IWYUPragmaKeep[] = "// IWYU pragma: keep"; -const char IWYUPragmaExport[] = "// IWYU pragma: export"; -const char

[PATCH] D135314: [clangd] Avoid scanning up to end of file on each comment!

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

[PATCH] D133415: [clangd] Fix non-idempotent cases of canonicalRenameDecl()

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rG5f7f4846e826: [clangd] Fix non-idempotent cases of canonicalRenameDecl() (authored by sammccall).

[PATCH] D133415: [clangd] Fix non-idempotent cases of canonicalRenameDecl()

sammccall marked an inline comment as done. sammccall added inline comments. Comment at: clang-tools-extra/clangd/unittests/RenameTests.cpp:1521 R"cpp( template class Foo { virtual void [[m]](); }; class Bar : Foo { void [[^m]]() override; };

[PATCH] D135231: [clangd] Don't clone SymbolSlab::Builder arenas when finalizing.

This revision was automatically updated to reflect the committed changes. Closed by commit rG6ea83fc98fd1: [clangd] Dont clone SymbolSlab::Builder arenas when finalizing. (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D135295: [clang][ExtractAPI] Don't print locations for anonymous tags

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. LG from my side assuming you're happy with the strings changing. In D135295#3837734 , @ributzka wrote: > This doesn't affect any tests? This

[PATCH] D135226: [clangd] Optimize Dex::generateProximityURIs().

sammccall added inline comments. Comment at: clang-tools-extra/clangd/index/dex/Dex.cpp:358 +// Returns^ +const char *findPathInURI(const char *S) { + // Skip over scheme. adamcz wrote: > Is there a reason why you're doing this with manual manipulation

[PATCH] D135226: [clangd] Optimize Dex::generateProximityURIs().

sammccall updated this revision to Diff 465492. sammccall added a comment. pointers -> StringRef Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D135226/new/ https://reviews.llvm.org/D135226 Files: clang-tools-extra/clangd/index/dex/Dex.cpp

[PATCH] D134685: Fix SourceManager::isBeforeInTranslationUnit bug with token-pasting

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rG41b51007e637: Fix SourceManager::isBeforeInTranslationUnit bug with token-pasting (authored by

[PATCH] D134618: [Syntax] Fix macro-arg handling in TokenBuffer::spelledForExpanded

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 rG67268ee11c22: [Syntax] Fix macro-arg handling in TokenBuffer::spelledForExpanded (authored by

[PATCH] D135258: [SourceManager] Improve getFileIDLoaded.

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Nice! Just a couple of comments where we could take the opportunity to clarify the existing code. Optional. Comment at: clang/lib/Basic/SourceManager.cpp:874 +

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

sammccall added a comment. I just landed that change to USR generation as 20c9ac29250493f5e0a3791dc1e5e9114ff0dc6e , so there should now be no USR changes (just some changes to debug representations that happen to be in some

[PATCH] D135191: [Index] USRGeneration doesn't depend on unnamed.printName() => ''. NFC

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG20c9ac292504: [Index] USRGeneration doesnt depend on unnamed.printName() = . NFC (authored by sammccall). Repository:

[PATCH] D135245: [clang][Tooling] Move STL recognizer to its own library

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang-tools-extra/clangd/CMakeLists.txt:163 clangToolingInclusions + clangToolingInclusionsSTL clangToolingSyntax

[PATCH] D134942: [Lex] Simplify and cleanup the updateConsecutiveMacroArgTokens implementation.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Lex/TokenLexer.cpp:1038 + for (Token& T : Partition) { +SourceLocation::IntTy RelativeOffset = T.getLocation().getRawEncoding() - +

[PATCH] D135132: [SourceManager] Improve getFileIDLocal.

sammccall accepted this revision. sammccall added inline comments. This revision is now accepted and ready to land. Comment at: clang/lib/Basic/SourceManager.cpp:807 } // Find the FileID that contains this. "I" is an iterator that points to a Just

[PATCH] D135231: [clangd] Don't clone SymbolSlab::Builder arenas when finalizing.

sammccall added a comment. My first take on this was to keep track in various ways of whether GC was required (statically known at callsite, or have builder track whether symbols were overwritten). However these failed to identify a bunch of cases where the arena wasn't shrinking, and this

[PATCH] D135231: [clangd] Don't clone SymbolSlab::Builder arenas when finalizing.

sammccall created this revision. sammccall added a reviewer: adamcz. 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] D135226: [clangd] Optimize Dex::generateProximityURIs().

sammccall created this revision. sammccall added a reviewer: adamcz. 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] D20401: [Lexer] Don't merge macro args from different macro files

sammccall added a comment. Started a thread: https://discourse.llvm.org/t/macro-performance-lexer-and-sourcemanager/65713 Repository: rL LLVM CHANGES SINCE LAST ACTION https://reviews.llvm.org/D20401/new/ https://reviews.llvm.org/D20401 ___

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

sammccall added a comment. In D134813#3834776 , @aaron.ballman wrote: > Thank you for this! I applied D135191 over > the top of my changes here and ran the tests to get all the new failures with > the changes, then I

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

sammccall added a comment. In D134813#3834613 , @sammccall wrote: > Changing USR generation to not rely on this detail seems easier, I can take a > stab at this. Seems trivial: https://reviews.llvm.org/D135191 If there are still diffs in USR tests

[PATCH] D135191: [Index] USRGeneration doesn't depend on unnamed.printName() => ''. NFC

sammccall created this revision. sammccall added a reviewer: aaron.ballman. Herald added a subscriber: arphaman. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. This prepares for printName() to print

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

sammccall added a comment. In D134813#3834540 , @aaron.ballman wrote: > In D134813#3834496 , @sammccall > wrote: > >> Sorry, Monday was a holiday here... > > No worries, I hope you had a good holiday! Thanks

[PATCH] D135132: [SourceManager] Improve getFileIDLocal.

sammccall added a comment. Nice! Comment at: clang/lib/Basic/SourceManager.cpp:796 + // larger than SLocOffset. + const SrcMgr::SLocEntry *I = LocalSLocEntryTable.end(); + // LessIndex - This is the lower bound of the range that we're searching. hmm, while

[PATCH] D134942: [Lex] Simplify and cleanup the updateConsecutiveMacroArgTokens implementation.

sammccall added a comment. Thanks, I think this is worthwhile for the simpler code, better (non-lying) comments, avoiding arg-evaluation-order stuff. -0.05% time and -0.5% SLocEntries is interesting too! Clearly not earth-shattering but the the offset table idea gives us a lead to follow. I do

[PATCH] D134813: Properly print unnamed TagDecl objects in diagnostics

sammccall added a comment. Sorry, Monday was a holiday here... I don't think unconditionally including the filename in printQualifiedName() is great for tools, it can be unreasonably long and is generally just noise when shown in the context of that file. I'm surprised that USR generation +

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

sammccall updated this revision to Diff 464118. sammccall added a comment. Rebase on top of using enum parsing changes, store 'enum' location separately. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D134303/new/ https://reviews.llvm.org/D134303

[PATCH] D20401: [Lexer] Don't merge macro args from different macro files

sammccall added a comment. Thanks Nick for the info! No kernel experience here, so if you have any particular suggestions about how to measure the workload you care about it'd be much appreciated (e.g. are particular files that are slow enough to measure in isolation, or is it better to do a

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

sammccall added a comment. In D134303#3823620 , @urnathan wrote: > Does this still apply with the fix for dr2621 landed? Not cleanly, as getType already creates the ElaboratedTypeLoc for the qualifier but without the `enum` keyword. Wrapping it with a

[PATCH] D20401: [Lexer] Don't merge macro args from different macro files

sammccall added subscribers: hokein, sammccall. sammccall added a comment. Herald added a project: All. In D20401#2770059 , @nickdesaulniers wrote: > I know this was sped up slightly in 3339c568c43e4644f02289e5edfc78c860f19c9f, > but this change makes

[PATCH] D133468: [clang] Implement divergence for TypedefType and UsingType

sammccall accepted this revision. sammccall added a comment. Sorry about the delay, I've been juggling too many things :-( TL;DR: yes, I think it's reasonable. I think the implementation complexity is justified by what we get. Thanks for explaining! I think to minimize interface complexity, we

[PATCH] D134728: [clangd] Add highlighting modifiers "constructor" and "destructor"

sammccall added a comment. TL;DR: let's go with this patch as-is rather than borrowing problems from the future. This enum isn't growing very rapidly. In D134728#3822775 , @ckandeler wrote: > In D134728#3822653

[PATCH] D130863: [clangd] Make git ignore index directories

sammccall added a comment. @sums I think this just needs minor comments addressed and can be landed. Are you still interested in finishing it? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D130863/new/ https://reviews.llvm.org/D130863

[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. This is fantastic, I'm really not sure how I missed it, sorry :-( Comment at: clang-tools-extra/clangd/SemanticHighlighting.cpp:857

[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

sammccall added a comment. I think this patch is great apart from the testing issue. (I do see the value in a good end-to-end test for the exact bug being fixed, but would also be fine with this being a unit-test of CommandMangler with the query driver part mocked out)

[PATCH] D133756: [clangd] Introduce CompileCommandsAdjuster

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Sorry about the long turnaround; have had some less-fun-than-code things to deal with :-) Thanks as always for patience... Main thing I'd still like is to split into two separate

[PATCH] D134384: [clangd] Add support for HeaderInsertion in .clangd config file

sammccall added a comment. Thanks, this is a great idea. It also opens the door to suppressing includes of particular files later (e.g. by regex) with another config option, though we need to be careful of performance there. Comment at: clang-tools-extra/clangd/Config.h:27

[PATCH] D134728: [clangd] Add highlighting modifiers "constructor" and "destructor"

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Hmm, semantic tokens doesn't really provide a good way of "subclassing" kinds, I suppose modifiers are the best we have. It doesn't scale very well though: we're limited to 30 modifiers

[PATCH] D134827: [clangd] Avoid recursion on UnresolvedUsingValueDecl during semantic highlighting

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Based on the stacktrace, I came up with this crashing example, maybe it's a usable testcase? template class X { template class Y { using Y<0>::xxx; }; }; (not currently

[PATCH] D134685: Fix SourceManager::isBeforeInTranslationUnit bug with token-pasting

sammccall added a comment. > However I tried to avoid mixing these with subtle behavior changes, and will > send a followup instead. D134694 if you're interested. I guess I should try to get performance measures though... Repository: rG LLVM Github

[PATCH] D134694: Clean up isBeforeInTranslationUnit cache and add eviction.

sammccall created this revision. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Previously the first 300 entries would stay around forever, and any new queries would thrash on entry 301. The cache

[PATCH] D134685: Fix SourceManager::isBeforeInTranslationUnit bug with token-pasting

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

[PATCH] D134243: Don't crash when code completing `using enum ^Foo`.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rG30b676ac5f30: Dont crash when code completing `using enum ^Foo`. (authored by sammccall). Changed prior

[PATCH] D134618: [Syntax] Fix macro-arg handling in TokenBuffer::spelledForExpanded

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. A few cases were not handled correctly. Notably: #define ID(X) X

[PATCH] D134379: [clangd] IncludeCleaner: handle using namespace

sammccall added a comment. Is test.h meaningfully used in that example? Yes, the code is going to fail to compile without it, but it seems like the "spirit" of IWYU would say to delete both the include and the using directive. My concern about marking it used is that namespaces are typically

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

sammccall added a comment. In D134303#3805687 , @urnathan wrote: > AFAICT the UsingDecl doesn't capture the NestedNameSpecifier location We have `NestedNameSpecifierLoc UsingDecl::QualifierLoc` already, maybe I'm misunderstanding. > is UsingEnumDecl

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

sammccall added a comment. I just saw this touches the same code as @urnathan's D134283 , specifically `Sema::ActOnUsingEnumDeclaration`. I'm happy to wait for that to land and then merge, I think these will play nicely together. The changes to the actual

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

sammccall created this revision. sammccall added reviewers: ilya-biryukov, urnathan. Herald added subscribers: kadircet, arphaman, martong. Herald added a reviewer: shafik. Herald added a project: All. sammccall requested review of this revision. Herald added projects: clang, clang-tools-extra.

[PATCH] D133468: [clang] Implement divergence for TypedefType and UsingType

sammccall added a comment. The descriptions here are pretty general and opaque nad it's hard to follow what this means in practice. It sounds like the motivating cases are to do with template instantiation - can we see them? e.g. code + proposed diagnostics before/after, or some other

[PATCH] D134243: Don't crash when code completing `using enum ^Foo`.

sammccall added a comment. In D134243#3802457 , @urnathan wrote: > Might be mooted by fixing https://github.com/llvm/llvm-project/issues/57659, > which I am working on Great! Agree that larger changes around using-enum parsing would probably end up

[PATCH] D134243: Don't crash when code completing `using enum ^Foo`.

sammccall added inline comments. Comment at: clang/lib/Parse/ParseDecl.cpp:4591 Actions.CodeCompleteTag(getCurScope(), DeclSpec::TST_enum); +DS.SetTypeSpecError(); return; this is a bit unusual, usually parse() functions just stop after hitting CC,

[PATCH] D134243: Don't crash when code completing `using enum ^Foo`.

sammccall created this revision. sammccall added a reviewer: ilya-biryukov. Herald added a subscriber: kadircet. Herald added a project: All. sammccall requested review of this revision. Herald added a project: clang. Herald added a subscriber: cfe-commits. Fixes

[PATCH] D133440: [clangd] Allow programmatically disabling rename of virtual method hierarchies.

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. sammccall marked an inline comment as done. Closed by commit rGe42441835827: [clangd] Allow programmatically disabling rename of virtual method hierarchies. (authored

[PATCH] D133982: [clangd] Improve inlay hints of things expanded from macros

This revision was automatically updated to reflect the committed changes. Closed by commit rG924974a3a13b: [clangd] Improve inlay hints of things expanded from macros (authored by sammccall). Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D133982/new/

[PATCH] D133982: [clangd] Improve inlay hints of things expanded from macros

sammccall added inline comments. Comment at: clang-tools-extra/clangd/InlayHints.cpp:281 - void addReturnTypeHint(FunctionDecl *D, SourceLocation Loc) { + void addReturnTypeHint(FunctionDecl *D, SourceRange Range) { auto *AT = D->getReturnType()->getContainedAutoType();

[PATCH] D133982: [clangd] Improve inlay hints of things expanded from macros

sammccall created this revision. sammccall added a reviewer: nridge. 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. When we aim

[PATCH] D133979: [clangd] XRef functions treat visible class definitions as primary.

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] D133968: [clangd] Enable standard library index by default.

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.

[PATCH] D133756: [clangd] Introduce CompileCommandsAdjuster

sammccall added inline comments. Comment at: clang-tools-extra/clangd/GlobalCompilationDatabase.h:165 +// process a file (possibly different from the one in the command). +class CompileCommandsAdjuster { +public: I have a couple of concerns with this interface:

[PATCH] D133757: [clangd] Turn QueryDriverDatabase into a CompileCommandsAdjuster

sammccall added a comment. Kadir and I discussed this a bunch today. FWIW I agree with this patch: sticking the query-driver step in the middle of the pipeline is the right way to solve these bugs. Putting it on the end and hoping to get away with it feels risky to me.

[PATCH] D133843: [clangd] Prefer definitions for gototype and implementation

sammccall added a comment. There's no test here, can you describe the cases you expect this to affect and why the new behavior is better? For types this seems doubly-dubious at first glance: - when we've decided to "prefer" a non-definition declaration, why make an exception here? - I'd

[PATCH] D133339: [clangd] Isolate logic for setting LSPServer options

sammccall added a comment. I like the change to initialize() a lot. Comment at: clang-tools-extra/clangd/ClangdLSPServer.h:64 + +/// Options used for diagnostics. +ClangdDiagnosticOptions DiagOpts; I don't really like making these options part of this

[PATCH] D133440: [clangd] Allow programmatically disabling rename of virtual method hierarchies.

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] D127082: [clangd] Add Macro Expansion to Hover

This revision was not accepted when it landed; it landed in state "Needs Review". This revision was automatically updated to reflect the committed changes. Closed by commit rG44bbf20965d2: [clangd] Add Macro Expansion to Hover (authored by daiyousei-qz, committed by sammccall). Changed prior to

[PATCH] D132867: [Clang] Use virtual FS in processing config files

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. Thanks! Comment at: clang/docs/ReleaseNotes.rst:87-88 `Issue 57387 `_. +- Fix clang not properly handling

[PATCH] D131465: C++/ObjC++: switch to gnu++17 as the default standard

sammccall added a comment. In D131465#3774276 , @aaron.ballman wrote: > In D131465#3772803 , @MaskRay wrote: > >> Update clang-tools-extra/clangd/unittests/SelectionTests.cpp @sammccall > > This one looks to

[PATCH] D133423: [clangd] Improve Selection testcase, pin to C++17

This revision was automatically updated to reflect the committed changes. Closed by commit rG8af74da5bdbd: [clangd] Improve Selection testcase, pin to C++17 (authored by sammccall). Changed prior to commit: https://reviews.llvm.org/D133423?vs=458445=458450#toc Repository: rG LLVM Github

[PATCH] D129683: [Sema] Move Diags.isIgnored() checks off hot paths, it's not free. NFC

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG897f3ddc6154: [Sema] Move Diags.isIgnored() checks off hot paths, its not free. NFC (authored by sammccall). Changed prior to commit:

[PATCH] D133423: [clangd] Improve Selection testcase, pin to C++17

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. 17 vs 14

[PATCH] D133415: [clangd] Fix non-idempotent cases of canonicalRenameDecl()

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

[PATCH] D132797: [clangd] Support renaming virtual methods

This revision was landed with ongoing or failed builds. This revision was automatically updated to reflect the committed changes. Closed by commit rG8019b46bc70b: [clangd] Support renaming virtual methods (authored by tom-anders, committed by sammccall). Changed prior to commit:

[PATCH] D132797: [clangd] Support renaming virtual methods

sammccall accepted this revision. sammccall added a comment. This revision is now accepted and ready to land. LG IIRC Tom doesn't have commit access, so I'll apply the last trivial changes and commit to avoid another round trip. (Please LMK if i'm wrong about this!) Comment

[PATCH] D131465: C++/ObjC++: switch to gnu++17 as the default standard

sammccall added a comment. Thanks for doing this, clangd change LGTM. Comment at: clang-tools-extra/clangd/unittests/SelectionTests.cpp:726 EXPECT_EQ("CXXConstructExpr", nodeKind(Str->Parent->Parent)); EXPECT_EQ(Str, >Parent->Parent->ignoreImplicit()) << "Didn't

[PATCH] D132867: [Clang] Use virtual FS in processing config files

sammccall added a comment. In D132867#3770898 , @sepavloff wrote: > It seems that compatibility issue can appear if someone use VFS for all files > but configuration one, which reside in real FS. It is inconvenient and hard > to maintain I agree, the

[PATCH] D127082: [clangd] Add Macro Expansion to Hover

sammccall added a comment. Sorry for the delay :-( This looks great! It looks like this is your first patch, so you don't have commit access. I can land the patch for you. Can you provide an email address for attribution? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION

[PATCH] D131066: [clangd] Add option to disable inlay hints for init lists when building array.

sammccall added a comment. We **have** been thinking about problems with inlay hints being verbose, and array initializers that are visible by default are clearly seen as spammy. Thanks for sending a patch. I filed https://github.com/clangd/clangd/issues/1277 about this just now and added some

[PATCH] D132797: [clangd] Support renaming virtual methods

sammccall added inline comments. Comment at: clang-tools-extra/clangd/refactor/Rename.cpp:548 +namespace { +void recursivelyInsertOverrides(const SymbolID , +llvm::DenseSet , sammccall wrote: > The fact that we only need to walk

[PATCH] D132797: [clangd] Support renaming virtual methods

sammccall added a comment. Sorry for the delay, this patch is really neat! The only serious thing is it uncovers an existing bug which asserts. As a potential new crash I think we should fix that - LMK whether you feel comfortable adding that fix in here or you'd like me to do it as a separate

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