Review Request 124200: Introduction of rename variable refactoring

Milian Wolff mail at milianw.de
Wed Jul 1 08:42:34 UTC 2015 


> On June 29, 2015, 2:27 p.m., 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.
> 
> Maciej Poleski wrote:
>     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.

no, just don't commit these changes. `git add -p` allows you to select change hunks selectively, just skip this file there then.


> On June 29, 2015, 2:27 p.m., 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.
> 
> Maciej Poleski wrote:
>     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.

but why should this pass a Q_ASSERT? I really dislike this way of programming. Each commit should incrementally improve on the previous iteration. And every commit on its own should compile and "run", i.e. not crash. It does not have to do anything, but better no code than intentionally broken hacks such as this one.

Not only could it potentially slip into future code, but I also cannot read your mind. Thus, whenever you present me such code for review, I'll should out that this is bad style and we'll waste time in the discussion period then.


> On June 29, 2015, 2:27 p.m., 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?
> 
> Maciej Poleski wrote:
>     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

Yes, but as I told you elsewhere, you must not configure synchronously, as that freezes the application. In general this code path is not the correct place to manipulate the projects at all. If at all, request a "ensureConfigured" or just check a "isConfigured" on the project. This would mean we need to add such API to the project managers.


> On June 29, 2015, 2:27 p.m., 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.
> 
> Maciej Poleski wrote:
>     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?

the issue you describe with finished not getting dispatched properly sounds like a bug. Can you create a simplified test case (unrelated to the work in this branch) and show it to us? Or at least show us the code you tried to write here. Then we could debug it together and find a way to fix it. Potentially, your code blocked and did not hand over work to the eventloop, thereby blocking the configure job from executing.

And the signals in IProjectBuilder are severly broken, looking at it now. They should a) have the KDevelop:: QID in their arguments for old-style signal/slot connections to work properly (is that what you tried?), and b) most of them never get emitted if I'm not mistaken. So you'll have to fix this first (in separate patches and review requests!).


> On June 29, 2015, 2:27 p.m., 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.
> 
> Maciej Poleski wrote:
>     I guess you mean `RefactoringInfo::~RefactoringInfo() = default`?
>     
>     But why inline here is undesirable? It's "body" should be more or less "no op".

Yes, `default` - my bad. And it should not be inlined to ensure the vtable doesn't get inlined and then put into every translation unit that uses this header.


> On June 29, 2015, 2:27 p.m., Milian Wolff wrote:
> > refactoring/renamevardeclrefactoring.cpp, line 101
> > <https://git.reviewboard.kde.org/r/124200/diff/1/?file=381622#file381622line101>
> >
> >     QStringLiteral
> 
> Maciej Poleski wrote:
>     It may be desirable to localize this string later. `KLocalizedString`?

yep good idea.


- Milian


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


On June 27, 2015, 10:10 p.m., Maciej Poleski wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/124200/
> -----------------------------------------------------------
> 
> (Updated June 27, 2015, 10:10 p.m.)
> 
> 
> 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/20150701/26432044/attachment-0001.html>

More information about the KDevelop-devel mailing list