Review Request 124200: Introduction of rename variable refactoring

Mon Jun 29 14:27:53 UTC 2015

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

interesting to see the first active refactoring action implemented! there are some small issues, and I have also added comments here now that should be targeted at https://git.reviewboard.kde.org/r/123963/diff/6 - sorry. Hope you can sort that out by yourself and update both review requests as appropriate. ReviewBoard is really bad at showing patches individually...

Rock on!

.reviewboardrc 
<https://git.reviewboard.kde.org/r/124200/#comment56185>

    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.

CMakeLists.txt (line 35)
<https://git.reviewboard.kde.org/r/124200/#comment56187>

    put this comment above the conditional for `CMAKE_HOST_UNIX`.

CMakeLists.txt (line 36)
<https://git.reviewboard.kde.org/r/124200/#comment56188>

    b*e*cause

refactoring/documentcache.h (line 45)
<https://git.reviewboard.kde.org/r/124200/#comment56189>

    I still don't like handing out `const&`. The correct way is to bind to a `const &` at the callee site, imo. If you don't do that, it will still copy.

refactoring/interface.cpp (line 51)
<https://git.reviewboard.kde.org/r/124200/#comment56201>

    use `i18n` from `KLocalizedString`

refactoring/interface.cpp (line 164)
<https://git.reviewboard.kde.org/r/124200/#comment56200>

    please don't. either use Q_UNREACHABLE or return a nullptr.

refactoring/kdevrefactorings.cpp (line 52)
<https://git.reviewboard.kde.org/r/124200/#comment56190>

    alread*y*

    and it's a simple foreach, couldn't you add it directly?

refactoring/kdevrefactorings.cpp (line 103)
<https://git.reviewboard.kde.org/r/124200/#comment56191>

    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.

refactoring/nooprefactoring.cpp (line 35)
<https://git.reviewboard.kde.org/r/124200/#comment56193>

    style:

    `const QUrl &sourceFile`

    also for the cursor below

refactoring/refactoring.h (line 51)
<https://git.reviewboard.kde.org/r/124200/#comment56196>

    style: `const QUrl &sourceFile, const KTextEditor::Cursor &position`

refactoring/refactoringinfo.h (line 35)
<https://git.reviewboard.kde.org/r/124200/#comment56194>

    deinline it by writing

        RefactoringInfo::~RefactoringInfo() = delete;

    into the .cpp file.

refactoring/renamevardeclrefactoring.h (line 37)
<https://git.reviewboard.kde.org/r/124200/#comment56195>

    remove empty line

refactoring/renamevardeclrefactoring.cpp (line 77)
<https://git.reviewboard.kde.org/r/124200/#comment56197>

    constify the `newName` and use i18n from `KLocalizedString` for the strings.

refactoring/renamevardeclrefactoring.cpp (line 101)
<https://git.reviewboard.kde.org/r/124200/#comment56198>

    QStringLiteral

refactoring/renamevardeclrefactoring.cpp (line 104)
<https://git.reviewboard.kde.org/r/124200/#comment56199>

    style: `const ... &result`

- Milian Wolff

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/20150629/2fe5a74a/attachment-0001.html>