[KDE/Mac] Review Request 128272: support -iframework and -F header search path options

Milian Wolff mail at milianw.de
Mon Jun 27 08:59:31 UTC 2016 

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




languages/clang/clangparsejob.cpp (line 167)
<https://git.reviewboard.kde.org/r/128272/#comment65452>

    looking at this code, I think we now need an API that  does this in bulk, i.e.
    
        const auto info& = IDAIM::manager()->info(file);
        /* info.includes, info.frameworkDirectories, info.defines */
    
    if you want to, you could tackle this as a follow-up (don't do it in this change!)



languages/clang/clangparsejob.cpp (line 168)
<https://git.reviewboard.kde.org/r/128272/#comment65451>

    sorry for the go-around, but please keep it all consistent. If you use `Directories` as suffix in the KDevplatform API, also use it here please



languages/clang/duchain/clangparsingenvironment.h (line 35)
<https://git.reviewboard.kde.org/r/128272/#comment65453>

    ? don't ifdef this



languages/clang/duchain/clangparsingenvironment.cpp (line 29)
<https://git.reviewboard.kde.org/r/128272/#comment65454>

    it's the ctor, you don't need to clear anything.



languages/clang/duchain/clangparsingenvironment.cpp (line 30)
<https://git.reviewboard.kde.org/r/128272/#comment65455>

    ifdef this part only, but use an initializer and `QStringLiteral` instead. Actually, don't initialize it here - these defaults should be put into the IncludesAndDefinesManager



languages/clang/duchain/clangparsingenvironment.cpp (line 85)
<https://git.reviewboard.kde.org/r/128272/#comment65456>

    please share this code with the one above, introduce a templated function here and call it from both methods.



languages/clang/duchain/parsesession.cpp (line 124)
<https://git.reviewboard.kde.org/r/128272/#comment65457>

    again, share code



languages/plugins/custom-definesandincludes/CMakeLists.txt (line 39)
<https://git.reviewboard.kde.org/r/128272/#comment65458>

    unrelated change, please make sure this is a separate commit (feel free to push it directly)



languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp (line 155)
<https://git.reviewboard.kde.org/r/128272/#comment65460>

    the default values that you added into kdev-clang come to mind.



languages/plugins/custom-definesandincludes/definesandincludesmanager.cpp (line 197)
<https://git.reviewboard.kde.org/r/128272/#comment65459>

    ?



projectmanagers/cmake/cmakemanager.cpp (line 128)
<https://git.reviewboard.kde.org/r/128272/#comment65461>

    I'd rename it to hasBuildInfo or similar and return true if any info is available (i.e. the current implementation).



projectmanagers/custommake/custommakemanager.cpp (line 84)
<https://git.reviewboard.kde.org/r/128272/#comment65462>

    again, share code



projectmanagers/custommake/makefileresolver/makefileresolver.cpp (line 258)
<https://git.reviewboard.kde.org/r/128272/#comment65463>

    also put this into a function or lambda and share code



projectmanagers/qmake/qmakemanager.cpp (line 433)
<https://git.reviewboard.kde.org/r/128272/#comment65464>

    ? What do you want to investigate - do you want to return a non-empty set?
    
    If so, make it `// TODO: implement`. Note though that you'd need to parse the linker flags apparently:
    
    http://doc.qt.io/qt-5/qmake-platform-notes.html#using-frameworks


- Milian Wolff


On June 25, 2016, 1:02 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/128272/
> -----------------------------------------------------------
> 
> (Updated June 25, 2016, 1:02 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDevelop.
> 
> 
> Repository: kdevelop
> 
> 
> Description
> -------
> 
> This is a draft implementation for parser support of the `-iframework dir` and `-F dir` compiler arguments. On OS X these are the framework equivalents of `-isystem` and `-I` respectively, telling the compiler and/or linker where to find framework bundles.
> 
> I started out making the new code available on OS X only but that introduces a lot of #ifdefs for probably little benefit. On the contrary, clang supports the arguments on Linux too, presumably because clang is a functional cross-compiler that can generate Darwin Mach-O object files on Linux too.
> 
> For the 1st approach I propose to parse the framework directories, adding the effective header directories of the individual frameworks as if they were added explicitly. The framework directories are also added to a new list in the result structure. I presume that this is a prerequisite for adding them to the (lib)clang arguments of the clang parser.
> 
> 
> Diffs
> -----
> 
>   languages/clang/clangparsejob.cpp 8375eb5 
>   languages/clang/duchain/clangparsingenvironment.h c689132 
>   languages/clang/duchain/clangparsingenvironment.cpp b515037 
>   languages/clang/duchain/parsesession.cpp aae0661 
>   languages/plugins/custom-definesandincludes/CMakeLists.txt 5a6c5b7 
>   languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.h 7a5184f 
>   languages/plugins/custom-definesandincludes/compilerprovider/compilerprovider.cpp 24e532a 
>   languages/plugins/custom-definesandincludes/definesandincludesmanager.h 6d0d210 
>   languages/plugins/custom-definesandincludes/definesandincludesmanager.cpp ebceb4d 
>   languages/plugins/custom-definesandincludes/idefinesandincludesmanager.h 5da71f2 
>   projectmanagers/cmake/cmakeimportjsonjob.cpp f064647 
>   projectmanagers/cmake/cmakemanager.h 3096b7d 
>   projectmanagers/cmake/cmakemanager.cpp 5c15e2f 
>   projectmanagers/cmake/cmakeprojectdata.h 60e8773 
>   projectmanagers/custom-buildsystem/custombuildsystemplugin.h 372b283 
>   projectmanagers/custom-buildsystem/custombuildsystemplugin.cpp b04647e 
>   projectmanagers/custommake/custommakemanager.h 33c2997 
>   projectmanagers/custommake/custommakemanager.cpp e2ce943 
>   projectmanagers/custommake/makefileresolver/makefileresolver.h debe977 
>   projectmanagers/custommake/makefileresolver/makefileresolver.cpp 97973d4 
>   projectmanagers/custommake/makefileresolver/tests/test_custommake.h 3ad0f36 
>   projectmanagers/custommake/makefileresolver/tests/test_custommake.cpp 368e83e 
>   projectmanagers/qmake/qmakemanager.h e5e3266 
>   projectmanagers/qmake/qmakemanager.cpp 123b474 
> 
> Diff: https://git.reviewboard.kde.org/r/128272/diff/
> 
> 
> Testing
> -------
> 
> the unittest works as expected on OS X.
> 
> 20160625: the patch works as expected on OS X. The `-iframework /opt/local/libexec/qt5/Library/Frameworks` argument added by cmake to each compiler invocation is detected and put to use; Qt header files are found in the frameworks without a wrapper Qt5 header directory (`/opt/local/include/qt5`) added to the header search path. Header files from the system SDKs are found too
> 
> 
> File Attachments
> ----------------
> 
> the real companion patch for kdevplatform
>   https://git.reviewboard.kde.org/media/uploaded/files/2016/06/22/67189f62-ec2c-4797-a315-cafc44fbbb6d__patch-support-kdevp-frameworks.diff
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20160627/351cc321/attachment-0001.html>

More information about the kde-mac mailing list