Page MenuHomePhabricator

[clangd] Trivial setter support when moving items to fields
ClosedPublic

Authored by njames93 on Sep 25 2020, 6:01 AM.

Details

Summary

Extend the Trivial setter documentation to support cases where the value is moved into a field using std::move.

Diff Detail

Event Timeline

njames93 created this revision.Sep 25 2020, 6:01 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 25 2020, 6:01 AM
njames93 requested review of this revision.Sep 25 2020, 6:01 AM
sammccall accepted this revision.Sep 25 2020, 6:41 AM

Nice, thanks!

clang-tools-extra/clangd/Hover.cpp
463

nit: you could skip this check if you like, the other variant isn't going to be on the RHS of an assignment :-)

470

I guess you want !ND->getIdentifier() || ND->getName() != "move" || ... to guard against it being a special name somehow - getName() asserts in that case.

This revision is now accepted and ready to land.Sep 25 2020, 6:41 AM
kadircet added inline comments.Sep 25 2020, 6:42 AM
clang-tools-extra/clangd/Hover.cpp
414

can you also update the docs?

468

nit: combine with the next condition, and perform the string comparison last, i.e.:

if(!ND || !ND->isInStd || ND->getName() != "move")

njames93 updated this revision to Diff 294357.Sep 25 2020, 9:57 AM
njames93 marked 2 inline comments as done.

Updated function doc
Fix potential assertion

clang-tools-extra/clangd/Hover.cpp
463

You never know what crazy code people could concoct. It could be argued that this has a performance win by easily filtering out some bad candidates quickly and avoid needing to run the (slightly) more expensive checks on the name later down the line :-)

468

Can you explain the reasoning for moving the comparison to the end?

kadircet accepted this revision.Tue, Sep 29, 12:28 PM
kadircet added inline comments.
clang-tools-extra/clangd/Hover.cpp
468

it was to skip a potentialyl expensive string comparison, but now that I look into isInStdNamespace it is also performing a string comparison in the end. so nvm.

This revision was landed with ongoing or failed builds.Tue, Sep 29, 1:51 PM
This revision was automatically updated to reflect the committed changes.