Review Request 119057: Split the IncludePathResolver into MakeFileResolver and NoProjectIncludePathsManager

[Sergey Kalinichev]

> On July 9, 2014, 9:26 p.m., Kevin Funk wrote:
> > languages/plugins/custom-definesandincludes/definesandincludesmanager.h, line 40
> > <https://git.reviewboard.kde.org/r/119057/diff/1/?file=285854#file285854line40>
> >
> >     Do we really need yet another interface here? Referring to "IDefinesAndIncludesConfigurationDialog".
> >     
> >     I don't like dynamic_cast'ing around to get basic functionality.
> 
> Milian Wolff wrote:
>     I agree, this is not required. Just put openConfigurationDialog into IDefinesAndIncludesManager. Btw, the file for IDefinesAndIncludesConfigurationDialog is missing in the uploaded changeset, is it not? Anyhow, by putting this one method into the manager itself you don't need a full new file and interface anyways. much simpler.

>I don't like dynamic_cast'ing around to get basic functionality.

Why? I can use static_cast's then...

>I agree, this is not required. Just put openConfigurationDialog into IDefinesAndIncludesManager.

What is wrong with it anyway? I'm trying to follow the ISP here. (http://en.wikipedia.org/wiki/Interface_segregation_principle)

>Btw, the file for IDefinesAndIncludesConfigurationDialog is missing in the uploaded changeset, is it not?

No, it's not. I put that interface into IDefinesAndIncludesManager instead, as it pretty simple and only IDAIM uses it.


- Sergey


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


On July 1, 2014, 3:55 p.m., Sergey Kalinichev wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/119057/
> -----------------------------------------------------------
> 
> (Updated July 1, 2014, 3:55 p.m.)
> 
> 
> Review request for KDevelop.
> 
> 
> Repository: kdevelop
> 
> 
> Description
> -------
> 
> This is needed as it will let us to get rid of the .kdev_include_paths related code from the kdev-clang. Also it'll make it easier to extract the MakeFileResolver in the future.
> 
> Also simplified the code.
> 
> 
> Diffs
> -----
> 
>   languages/cpp/CMakeLists.txt 48f12da 
>   languages/cpp/codegen/customincludepaths.h 1041682 
>   languages/cpp/codegen/customincludepaths.cpp 8e4e41e 
>   languages/cpp/codegen/ui/custom_include_paths.ui 37be9b5 
>   languages/cpp/codegen/unresolvedincludeassistant.h 60a048b 
>   languages/cpp/codegen/unresolvedincludeassistant.cpp 7c3bc49 
>   languages/cpp/cpplanguagesupport.h e1659ae 
>   languages/cpp/cpplanguagesupport.cpp 79530e9 
>   languages/cpp/includepathcomputer.h 61de3ec 
>   languages/cpp/includepathcomputer.cpp 61c3646 
>   languages/cpp/includepathresolver.h c9e9d7d 
>   languages/cpp/includepathresolver.cpp 3cffeb1 
>   languages/cpp/makefileresolver.h PRE-CREATION 
>   languages/cpp/makefileresolver.cpp PRE-CREATION 
>   languages/cpp/tests/CMakeLists.txt 68ba233 
>   languages/plugins/custom-definesandincludes/CMakeLists.txt 4dc013c 
>   languages/plugins/custom-definesandincludes/definesandincludesmanager.h f996a83 
>   languages/plugins/custom-definesandincludes/definesandincludesmanager.cpp 98f32fc 
>   languages/plugins/custom-definesandincludes/noprojectincludesanddefines/CMakeLists.txt PRE-CREATION 
>   languages/plugins/custom-definesandincludes/noprojectincludesanddefines/noprojectcustomincludepaths.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/noprojectincludesanddefines/noprojectcustomincludepaths.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/noprojectincludesanddefines/noprojectcustomincludepaths.ui PRE-CREATION 
>   languages/plugins/custom-definesandincludes/noprojectincludesanddefines/noprojectincludepathsmanager.h PRE-CREATION 
>   languages/plugins/custom-definesandincludes/noprojectincludesanddefines/noprojectincludepathsmanager.cpp PRE-CREATION 
>   languages/plugins/custom-definesandincludes/tests/definesandincludestest.h 1589b78 
>   languages/plugins/custom-definesandincludes/tests/definesandincludestest.cpp 9c421bb 
>   languages/plugins/custom-definesandincludes/tests/projectsgenerator.h b919ae5 
>   languages/plugins/custom-definesandincludes/tests/projectsgenerator.cpp 5d08a25 
> 
> Diff: https://git.reviewboard.kde.org/r/119057/diff/
> 
> 
> Testing
> -------
> 
> Added an unit test for NoProjectIncludePathsManager.
> 
> 
> File Attachments
> ----------------
> 
> kdevplatform.diff
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/07/01/39f48afe-f63e-46a1-ba1f-12a7a0d48a98__kdevplatform.diff
> before.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/07/01/bf114c23-763e-47b8-a153-70c57b14ecf7__before.png
> after.png
>   https://git.reviewboard.kde.org/media/uploaded/files/2014/07/01/b5cdd3d1-6b5e-4b6a-8075-21f8c9655f8b__after.png
> 
> 
> Thanks,
> 
> Sergey Kalinichev
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20140710/89528335/attachment.html>