[PATCH] D97803: [clangd] Overload bundles are only deprecated if each overloads is.

[Sam McCall via Phabricator via cfe-commits]

sammccall added a comment.
Herald added a project: clang-tools-extra.

Whoops, lost this patch...

In D97803#2602787 <https://reviews.llvm.org/D97803#2602787>, @kbobyrev wrote:

> So, if my understanding is correct, this will make the whole bundle non-deprecated if at least one overload is not deprecated?  This is probably an improvement over the existing behaviour.

Yes, exactly.

> However, do we maybe want to split the bundles into deprecated and non-deprecated groups?

I don't think splitting bundles over this is worthwhile.

- it's a distraction. The point of bundling is to hide some differences within an overload set, to let you see all the methods more clearly.
- the "deprecated" signal means "you don't want this". This is well-defined, and false, for a mixed bundle.



================
Comment at: clang-tools-extra/clangd/CodeComplete.cpp:410
+    if (Completion.Deprecated) {
+      Completion.Deprecated =
+          (C.SemaResult &&
----------------
kbobyrev wrote:
> The comment you added says "cleared" which means this should probably be `|=` rather than `=`, right?
> 
> Also, I this looks longer but probably more readable at least for me.
> 
> Also, the sources might be `SemaResult`, `IndexResult` or `IdentifierResult`, right? :( Otherwise could've been `Completion.Deprecated |= C.SemaResult ? C.SemaResult->Availability == CXAvailability_Deprecated : C.IndexResult->Flags & Symbol::Deprecated;`
> The comment you added says "cleared" which means this should probably be |= rather than =, right?

No, `Deprecated` *starts out true* and gets set to false (cleared) if we see any non-deprecated entry. (computes AND)

Your version assumes it starts out false and sets it if we see any deprecated entry. (computes OR).

I agree the OR version reads better - it's wrong though :-)


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D97803/new/

https://reviews.llvm.org/D97803