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

[René J.V. Bertin]

> On June 22, 2016, 10:38 p.m., Milian Wolff wrote:
> > ok, but you are still missing the defines/includes manager changes as well as forwarding the required stuff from the project manager interfaces

slowly getting there ... what we have here and now was the trivial part...

For now I hope that I can do with adding only frameworkDirs() and frameworkDirsInBackground() methods to IDefinesAndIncludesManager::Provider and IDefinesAndIncludesManager::backgroundProvider().

BTW it looks like the QMake manager doesn't use the MakeFileResolver at all? It also appears that I'll need to add a `frameworkDirectories()` method to IBuildSystemManager (which is in kdevplatform), is that correct?


> On June 22, 2016, 10:38 p.m., Milian Wolff wrote:
> > projectmanagers/custommake/makefileresolver/makefileresolver.h, line 45
> > <https://git.reviewboard.kde.org/r/128272/diff/5/?file=469744#file469744line45>
> >
> >     rename to frameworks?

I don't like that because this list holds directories that contain framework bundles; it's not a list of framework bundles. If you call clang with `-v` it prints things like 
```
 /System/Library/Frameworks (framework directory)
 /Library/Frameworks (framework directory)
```

that's what I based the variable name on. I call them `frameworkDirectories` or `frameworkPaths` if you prefer to avoid the abbreviation.


> On June 22, 2016, 10:38 p.m., Milian Wolff wrote:
> > projectmanagers/custommake/makefileresolver/makefileresolver.cpp, line 702
> > <https://git.reviewboard.kde.org/r/128272/diff/5/?file=469745#file469745line702>
> >
> >     are you sure that these paths cannot be quoted? are you sure they cannot be relative?
> >     
> >     I find it odd that you special case this. I'd expect that you'd simply do the quotation and relative path handling and finally do something like
> >     
> >     
> >         const auto& internedPath = internPath(path);
> >         const auto& type = match.captured(0);
> >         const auto isFramework = type.startsWith(QLatin1String("-iframework"))
> >             || type.startsWith(QLatin1String("-F"));
> >         if (isFramework) {
> >             ret.frameworks << internedPath;
> >         } else {
> >             ret.paths << internedPath;
> >         }

that's probably better indeed, thanks. I went a bit too fast when getting rid of the temporary list ... We can't use startsWith though or else we should first prune the leading whitespace. It's probably safe to use `match.captured(0).trimmed()`, don't you think?

About `internPath()`: if `m_pathCache` is being populated somewhere I'm not seeing it??


> On June 22, 2016, 10:38 p.m., Milian Wolff wrote:
> > projectmanagers/custommake/makefileresolver/makefileresolver.cpp, line 69
> > <https://git.reviewboard.kde.org/r/128272/diff/5/?file=469745#file469745line69>
> >
> >     rename to frameworks?

this one will be handled with a global search/replace if we agree on a better name, so dropping it for now.


> On June 22, 2016, 10:38 p.m., Milian Wolff wrote:
> > projectmanagers/custommake/makefileresolver/makefileresolver.cpp, line 708
> > <https://git.reviewboard.kde.org/r/128272/diff/5/?file=469745#file469745line708>
> >
> >     broken indentation

Indeed, the file should use 4 spaces like the rest of the project ^^

KDevelop doesn't have support for per-file tab-width settings, or does it?


- René J.V.


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


On June 22, 2016, 8:50 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 22, 2016, 8:50 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
> -----
> 
>   projectmanagers/custommake/makefileresolver/makefileresolver.h 22f9dba 
>   projectmanagers/custommake/makefileresolver/makefileresolver.cpp ab069bb 
>   projectmanagers/custommake/makefileresolver/tests/test_custommake.h 3ad0f36 
>   projectmanagers/custommake/makefileresolver/tests/test_custommake.cpp 368e83e 
> 
> Diff: https://git.reviewboard.kde.org/r/128272/diff/
> 
> 
> Testing
> -------
> 
> the unittest works as expected on OS X.
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20160622/7308e6ff/attachment.html>