Page MenuHomePhabricator

clang-rename: fix renaming member functions
AbandonedPublic

Authored by vmiklos on Jul 11 2016, 1:15 PM.

Details

Reviewers
klimek
omtcyfz
Summary

Testcase by Kirill Bobyrev.

Diff Detail

Event Timeline

vmiklos updated this revision to Diff 63561.Jul 11 2016, 1:15 PM
vmiklos retitled this revision from to clang-rename: fix renaming member functions.
vmiklos updated this object.
vmiklos added a reviewer: klimek.
vmiklos added a subscriber: cfe-commits.

This one doesn't fix it though.

It only deals with declarations of overridden functions.

This is the test that still fails:

/ RUN: cat %s > %t.cpp
// RUN: clang-rename -offset=161 -new-name=boo %t.cpp -i --
// RUN: sed 's,//.*,,' %t.cpp | FileCheck %s
// XFAIL: *

class A {
public:
  virtual void foo() {}
};

class B : public A {
public:
  void foo() override {}  // OK: B::foo overrides A::foo
};

int main() {
  A a;
  a.foo();

  B b;
  b.foo();

  return 0;
}

// Use grep -FUbo 'Cla' <file> to get the correct offset of foo when changing
// this file.
vmiklos updated this revision to Diff 64025.Jul 14 2016, 12:16 PM

Forgot to add asserts for main() in the testcase.

vmiklos updated this revision to Diff 64027.Jul 14 2016, 12:19 PM
omtcyf0 edited edge metadata.Jul 15 2016, 11:45 PM

Hi!

Sorry for a late response.

The issue is that you only deal with the consequences of this "bug". Overridden functions simply have different USRs, so the solution is just to add relevant USRs. Trying to deal with it in USRLocFinder.cpp doesn't seem right; the chances of dealing with such issue correctly on that side are very small. I.e. it still doesn't pass the more complicated test I have in my patch (see below).

There's a patch I've been working on lately, it does exactly that. https://reviews.llvm.org/D22408 (it's from the other account I'll have to use from now, nvm)

omtcyf0 edited reviewers, added: omtcyfz; removed: omtcyf0.Jul 16 2016, 1:17 AM
vmiklos abandoned this revision.Jul 16 2016, 5:49 AM

I see your point, then let me abandon this.