Review Request 124200: Introduction of rename variable refactoring

Mon Jun 29 18:14:23 UTC 2015

> On Cze 29, 2015, 4:27 po południu, Milian Wolff wrote:
> > .reviewboardrc, line 1
> > <https://git.reviewboard.kde.org/r/124200/diff/1/?file=381602#file381602line1>
> >
> >     could you keep these changes local please? esp. the target_groups should not be changed, and others probably won't push to the tooling branch, esp. later when we merge your branch.

I don't understand. Am i to gitignore this file?
I try to keep these changes on separate branch not to make unwanted issues on master.

> On Cze 29, 2015, 4:27 po południu, Milian Wolff wrote:
> > refactoring/interface.cpp, line 164
> > <https://git.reviewboard.kde.org/r/124200/diff/1/?file=381611#file381611line164>
> >
> >     please don't. either use Q_UNREACHABLE or return a nullptr.

I used this constant because i wanted this pointer to pass Q_ASSERT test.

Anyway it is very likely, that the next commit will remove this hack at all.

> On Cze 29, 2015, 4:27 po południu, Milian Wolff wrote:
> > refactoring/kdevrefactorings.cpp, line 52
> > <https://git.reviewboard.kde.org/r/124200/diff/1/?file=381613#file381613line52>
> >
> >     alread*y*
> >     
> >     and it's a simple foreach, couldn't you add it directly?

The problem i have is with properly invoking "configure" step and handlig its result. But if project is already configured - i don't want to invoke "configure" again

> On Cze 29, 2015, 4:27 po południu, Milian Wolff wrote:
> > refactoring/kdevrefactorings.cpp, line 103
> > <https://git.reviewboard.kde.org/r/124200/diff/1/?file=381613#file381613line103>
> >
> >     I know you have a FIXME above, but I just wanted to say that this is a _very_ bad idea. You essentially block the whole GUI now whenever a project gets opened. Also, the CMake manager already configures itself as required, so you shouldn't need to worry about this.

The problem i have (with async) is that project is configured as i requested, but signal (`finished`) is emitted/dispatched *after* i request KDevelop to close (crashing the whole application at shutdown) and not triggering appropriate handler at time. I don't know why.

If CMake manager does this as necessary - that's great. There is only one slight problem: i cannot connect to `IProjectBuilder::configured` - Qt says (at runtime), that CMakeBuilder doesn't have this signal (and strangely it looks like indeed there is no such signal (at least it is not listed here: https://projects.kde.org/projects/extragear/kdevelop/kdevelop/repository/revisions/master/entry/projectbuilders/cmakebuilder/cmakebuilder.h nor connected here: https://projects.kde.org/projects/extragear/kdevelop/kdevelop/repository/revisions/master/entry/projectbuilders/cmakebuilder/cmakebuilder.cpp#L82)) even though it is pure virtual in its base class. Could you explain to me what is happening there?

> On Cze 29, 2015, 4:27 po południu, Milian Wolff wrote:
> > refactoring/refactoringinfo.h, line 35
> > <https://git.reviewboard.kde.org/r/124200/diff/1/?file=381619#file381619line35>
> >
> >     deinline it by writing
> >     
> >         RefactoringInfo::~RefactoringInfo() = delete;
> >     
> >     into the .cpp file.

I guess you mean `RefactoringInfo::~RefactoringInfo() = default`?

But why inline here is undesirable? It's "body" should be more or less "no op".

> On Cze 29, 2015, 4:27 po południu, Milian Wolff wrote:
> > refactoring/renamevardeclrefactoring.cpp, line 101
> > <https://git.reviewboard.kde.org/r/124200/diff/1/?file=381622#file381622line101>
> >
> >     QStringLiteral

It may be desirable to localize this string later. `KLocalizedString`?

- Maciej

-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/124200/#review81865
-----------------------------------------------------------

On Cze 28, 2015, 12:10 rano, Maciej Poleski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124200/
> -----------------------------------------------------------
> 
> (Updated Cze 28, 2015, 12:10 rano)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdev-clang
> 
> 
> Description
> -------
> 
> Introduced rename variable refactoring.
> 
> This patch provides preliminary support for variable rename refactoring.
> Currently only one project is supported.
> Currently it is required to save all opened documents (cache is not ready).
> Regeneration of compile_commands.json needs to be polished. How to get appropriate notification (IProjectBuilder::configured signal)? As a workaround i simply restarted KDevelop to get this regenerated.
> To use this refactoring right click on variable declaration (definition) and select "rename" refactoring.
> 
> 
> Full renaming support will consist of a few parts - one of them is renaming around VarDecl (this patch).
> Quite early, but i wanted to see that this work is going into some direction after month of coding of pure interfaces/details.
> 
> How to split diffs? I noticed that RBTool created diff consisting of all changes ignoring other pending request. I wanted only the last one, because earlier commits are subject of other review.
> 
> 
> Diffs
> -----
> 
>   .reviewboardrc cd1ef2aa08623d813c80b5c5275b2ad0e35ff40e 
>   CMakeLists.txt 875172a8407f4bd9faf330f032a280fa66c2b16f 
>   clangsupport.h 8ed1ec90bbbc41d7c7a94d926e0951c729a6194c 
>   clangsupport.cpp e22c55426a2e839ec11cbe0b2fe1e13721b0583a 
>   cmake/FindClang.cmake 6c9bd6ef0242319122dcc7e6fd6cea8d9f0cbfbb 
>   refactoring/CMakeLists.txt PRE-CREATION 
>   refactoring/documentcache.h PRE-CREATION 
>   refactoring/documentcache.cpp PRE-CREATION 
>   refactoring/interface.h PRE-CREATION 
>   refactoring/interface.cpp PRE-CREATION 
>   refactoring/kdevrefactorings.h PRE-CREATION 
>   refactoring/kdevrefactorings.cpp PRE-CREATION 
>   refactoring/kdevrefactorings_disabled.h PRE-CREATION 
>   refactoring/nooprefactoring.h PRE-CREATION 
>   refactoring/nooprefactoring.cpp PRE-CREATION 
>   refactoring/refactoring.h PRE-CREATION 
>   refactoring/refactoring.cpp PRE-CREATION 
>   refactoring/refactoringinfo.h PRE-CREATION 
>   refactoring/refactoringinfo.cpp PRE-CREATION 
>   refactoring/renamevardeclrefactoring.h PRE-CREATION 
>   refactoring/renamevardeclrefactoring.cpp PRE-CREATION 
>   refactoring/utils.h PRE-CREATION 
>   refactoring/utils.cpp PRE-CREATION 
> 
> Diff: https://git.reviewboard.kde.org/r/124200/diff/
> 
> 
> Testing
> -------
> 
> Manual testing on very small code bases as cache and compile_commands.json regeneration is not fully functional.
> 
> 
> Thanks,
> 
> Maciej Poleski
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20150629/401c2d06/attachment-0001.html>