Review Request 128272: support -iframework and -F header search path options

[Milian Wolff]

> On June 22, 2016, 8: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
> 
> René J.V. Bertin wrote:
>     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?

yes, correct.


> On June 22, 2016, 8: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?
> 
> René J.V. Bertin wrote:
>     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.

use frameworkPaths then please


> On June 22, 2016, 8: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;
> >         }
> 
> René J.V. Bertin wrote:
>     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??

ah, I see - the regex has:

    "\s(?:--include-dir=|-I\s*|-isystem\s+|-iframework\s+|-F\s*)("

I think you want to make this parenthesis a capture (i.e. remove the ?:), then increment all indices into `.captured()` and then use `startsWith` on the then `.captured(1)`.

what is that with `m_patchCache` - you are again confusing me by jumping from one thing to another (both seemingly unrelated) without giving me enough context.


> On June 22, 2016, 8: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
> 
> René J.V. Bertin wrote:
>     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?

it does, using kate modelines


- Milian


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


On June 22, 2016, 6: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, 6: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/custommakemanager.h 33c2997 
>   projectmanagers/custommake/custommakemanager.cpp e2ce943 
>   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/kdevelop-devel/attachments/20160622/71a29368/attachment.html>