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
> 
> 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?
> 
> Milian Wolff wrote:
>     yes, correct.

The change is simple enough, but how do I add a patch for a KDevPlatform file to this ticket? As an attachment?


> 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;
> >         }
> 
> 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??
> 
> Milian Wolff wrote:
>     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.

That seems safe for the other options, do you confirm that?

The question about `m_pathCache` is just to understand if the `internPath` function indeed does what it's supposed to do.


> 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
> 
> 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?
> 
> Milian Wolff wrote:
>     it does, using kate modelines

Ah, right, of course. And adding one won't be frowned upon as an unrelated change?


- 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/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/beb65ecc/attachment-0001.html>