This is an archive of the discontinued LLVM Phabricator instance.

[clangd] Overload bundles are only deprecated if each overloads is.
ClosedPublic

Authored by sammccall on Mar 2 2021, 1:26 PM.

Diff Detail

Event Timeline

sammccall created this revision.Mar 2 2021, 1:26 PM
sammccall requested review of this revision.Mar 2 2021, 1:26 PM
sammccall updated this revision to Diff 327565.Mar 2 2021, 1:28 PM
sammccall edited the summary of this revision. (Show Details)

fix bug link

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. However, do we maybe want to split the bundles into deprecated and non-deprecated groups?

clang-tools-extra/clangd/CodeComplete.cpp
410

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;

Whoops, lost this patch...

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.
clang-tools-extra/clangd/CodeComplete.cpp
410

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 :-)

Herald added a project: Restricted Project. · View Herald TranscriptAug 10 2021, 6:55 AM
kbobyrev accepted this revision.Aug 12 2021, 2:57 AM

I see, thank you very much for the explanation!

clang-tools-extra/clangd/CodeComplete.cpp
410

Ahh, okay, makes sense, thank you!

Nit: I think the version I suggested (with fixes |= vs =) is somewhat easier to read and doesn't take much more space.

This revision is now accepted and ready to land.Aug 12 2021, 2:57 AM
sammccall added inline comments.Aug 12 2021, 12:21 PM
clang-tools-extra/clangd/CodeComplete.cpp
410

Done, though with &= instead of = otherwise the logic is subtly different (the second clobbers the first).
And because of &= we need an explicit cast to bool.

This revision was landed with ongoing or failed builds.Aug 12 2021, 2:46 PM
This revision was automatically updated to reflect the committed changes.