Review Request 119057: Split the IncludePathResolver into MakeFileResolver and NoProjectIncludePathsManager

[Milian Wolff]

> On July 9, 2014, 5: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.
> 
> Sergey Kalinichev wrote:
>     >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.

"...and only IDAIM uses it." <-- because of this, you should _not_ use an interface for this. An interface would make sense if other IDAIM implementation would exist, that would not implement the config dialog. That is not the case nor will it ever be. Adding yet another indirection is just useless bloat and complicates the code for no reason.

Note that this functionality belongs together, different interfaces would also make sense for stuff like our projectcontroller vs. plugincontroller vs. ... but not in this case. It's simply overdesigned the way it is right now.


- Milian


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


On July 1, 2014, 11:55 a.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, 11:55 a.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/b4859e14/attachment-0001.html>