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

[Milian Wolff]

On Thursday, June 23, 2016 10:04:28 AM CEST René J.V. Bertin wrote:
> > 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#file469745
> > > line702>> > 
> > >     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.

It is an empty Path if the path was not yet encountered, otherwise the code below goes into 
affect:

   if (ret.isEmpty() != path.isEmpty()) { 


-- 
Milian Wolff
mail at milianw.de
http://milianw.de
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-mac/attachments/20160623/215dda07/attachment-0001.html>