(pardon the interruption, some drive-by comments :))

• use isa_and_present
• indentation for tests

thanks, LG apart from the using decls

• address comments

thanks a lot for the review!

I am aware that this null checking at leaves are not considered a sustainable solution and I agree with the sentiment there. But we're seeing an increasing number of crashes in production on invalid code recently. Happy to take a different course if there are alternatives, but as also explained in D146426, the situation around parameter lists having nullptrs seem to be the state for a long time now, e.g:

template <typename T> auto x = [](__fp16) {};
decltype(x<int>);

is a reproducer that crashes even clang-12 due to a nullptr in the paremeter list. Surely it'd be better to fix this invariant, but I am afraid we don't know how to do that immediately and considering people have been dealing with this situation by adding null checks into the places that triggered crashes ever since, I'd like to move forward with this fix until someone can figure out the situation.

relanded in 35c2aac6e3957c2e82bf92269039fa02bab0e1d9

In D146490#4209495, @aganea wrote:

Fair enough. There are several choices forward: either we mark the issue as "Will Not Fix" or I can try only scoping this patch to only keep the handle open for network drives/paths. Any other suggestions?

To fix the issue, we keep the file handles open during the lifetime of their corresponding UniqueID instances. Since handles will live longer now, this requires particular attention when performing some file actions, such as file deletions.

Disable msvc compatibility to enable typo-correction on windows.

In D145843#4207101, @nridge wrote:

My understanding is that a more elaborate configuration scheme has been proposed in https://github.com/clangd/clangd/issues/1367, and the feedback there was (quoting Sam from this comment):
The approach taken in this patch seemed to me to be in line with this direction of a "simple config-based solution".

as discussed offline, this feels a little fishy and we should probably try and not put nulls into the parameter lists at all (and mark the functiontype as invalid instead), but since i don't know how to do that change myself it doesn't feel fair to ask it from you :)
LG and addresses a common crash we see on clangd, so let's ship it.

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

Style:
  IncludeInsertion:
      Directory: foo/
      Delimeter: <
   ... More IncludeInsertion customizations.

thanks!

Hi @owenpan, this seems to be crashing for:

struct Foo { operator enum foo{} };

FWIW, I believe this patch does the right thing by marking the DeducedTemplateSpecializationType as using. It's explicitly introduced into the global namespace through the using decl, and even before 3e78fa860235431323aaf08c8fa922d75a7cfffa we weren't marking them as such.

• Rebase
• Prevent unnecessary copies

• Use collectPragmaMarksCallback

Use std::copy

thanks!

thanks, lgtm!

• Disable special members tweaks on unions

we can also handle them through the stdlib symbol mappings, see https://github.com/llvm/llvm-project/issues/61373

i'd still merge this with the previous patch, as all of this is dead code after config option deletion. so it'd be better to just revert a single patch if we want to restore the old behavior, rather than two.

@kadircet it is obvious that there is something in this diff that causes this hesitancy in accepting it. I'm ready to keep iterating on the solution but I need a clue what needs the improvement. Please comment.

thanks for bearing with me, let's ship it!

i agree with Sam's concerns here. clangd isn't designed to be consumed as a library, but rather as a binary through LSP. increasing surface are here and letting people build applications on top of clangd internals would create extra maintenance burden that we're not equipped to support.

thanks, looks great!

thanks, LGTM!

oh right, i thought these were also provided by multiple headers, but looks like they're only provided by cstdlib, thanks!

this fixes the abs in the mentioned issue, but leaves the friends out :( those are specifically std::labs, std::llabs, std::imaxabs

thanks! looks amazing, we're missing a little bit of test coverage though

thanks!

adding Chandler, as he's the code owner here. I'll let this patch sit for a couple (business) days to make sure people with more (historical) context can make comments