Review Request 128272: support -iframework and -F header search path options
René J.V. Bertin
rjvbertin at gmail.com
Thu Jun 23 10:04:28 UTC 2016
> 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.
>
> René J.V. Bertin wrote:
> 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.
>
> Milian Wolff wrote:
> Yes, confirmed.
>
> Regarding m_pathCache:
>
> Path& ret = m_pathCache[path];
>
> That inserts the path if it does not exist and returns a reference to it, please lookup the QHash API documentation for that.
Ah, right: *If the hash contains no item with the key, the function inserts a default-constructed value into the hash with the key*. I don't recall that the comparable stl classes work that way, but the documentation on the "default-constructed values" also states that *For most value types, this simply means that a value is created using the default constructor (e.g. an empty string for QString).* I'll take your word on it that `ret` is not an empty QString in this case.
- René J.V.
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/128272/#review96800
-----------------------------------------------------------
On June 23, 2016, 12:56 a.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 23, 2016, 12:56 a.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/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/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 22f9dba
> projectmanagers/custommake/makefileresolver/makefileresolver.cpp ab069bb
> 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.
>
>
> 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/kdevelop-devel/attachments/20160623/3cdbff2a/attachment-0001.html>
More information about the KDevelop-devel
mailing list