Page MenuHomePhabricator

[clangd] Delete default arguments while moving functions out-of-line
ClosedPublic

Authored by kadircet on Dec 9 2019, 1:41 AM.

Details

Summary

Only function declarations should have the default arguments.

This patch makes sure we don't propogate those arguments to out-of-line
definitions.

Fixes https://github.com/clangd/clangd/issues/221

Diff Detail

Event Timeline

kadircet created this revision.Dec 9 2019, 1:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2019, 1:41 AM

Build result: pass - 60555 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

ilya-biryukov added inline comments.Dec 9 2019, 2:13 AM
clang-tools-extra/clangd/unittests/TweakTests.cpp
1976

NIT: missing full period at the end of the comment

kadircet updated this revision to Diff 232790.Dec 9 2019, 2:57 AM
  • Add . after comment and fix indentation.
kadircet marked an inline comment as done.Dec 9 2019, 2:58 AM

Build result: pass - 60555 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

hokein added inline comments.Dec 9 2019, 4:45 AM
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
190

just curious what's the getDefaultArgRange for case like void foo(int a = 5), is it just [[5]]?

195

The approach seems fail on the following case, StartLoc will point to Foo.

class Bar { void func(int); };
void bind(const char*, void(Bar::*Foo)(int) = nullptr)
kadircet updated this revision to Diff 232823.Dec 9 2019, 5:32 AM
kadircet marked 4 inline comments as done.
  • Make use of token buffer instead of trying to go-over identifier name.
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
190

yes it is the range of the initializer.

195

ah, right. switching to a similar strategy as D71188 to find first = before initializer.

Build result: pass - 60555 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

hokein accepted this revision.Dec 9 2019, 7:16 AM
hokein added inline comments.
clang-tools-extra/clangd/refactor/tweaks/DefineOutline.cpp
199

nit: llvm::find_if(llvm::reverse(Tokens), [](Tok) { return Toke.kind() == tok::equal; })

This revision is now accepted and ready to land.Dec 9 2019, 7:16 AM
kadircet updated this revision to Diff 232886.Dec 9 2019, 9:52 AM
kadircet marked an inline comment as done.
  • Address comments
This revision was automatically updated to reflect the committed changes.

Build result: pass - 60555 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt