<div dir="ltr">LOL, I just posted the review...<br></div><div class="gmail_extra"><br><div class="gmail_quote">On 28 January 2017 at 16:12, Stephen Kelly <span dir="ltr"><<a href="mailto:steveire@gmail.com" target="_blank">steveire@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Hi Shaheed,<br>
<br>
That's great, thanks for keeping us up to date with your progress! Looking<br>
forward to reviewing it.<br>
<br>
Thanks,<br>
<br>
Steve.<br>
<div class="HOEnZb"><div class="h5"><br>
<br>
Shaheed Haque wrote:<br>
<br>
> Hi Steve,<br>
><br>
> FWIW, I have rebased [1], debugged, bulk tested, and squashed to the<br>
> point where I can focus on adding to the unit tests for the new<br>
> functionality [2].<br>
><br>
> Basically, things took longer than expected because when I rebased, I<br>
> picked up your fix for the regression around reporting errors from the<br>
> clang compiler. That resulted in errors showing that I had some messed<br>
> up paths...and when I fixed that, all sort of objects were no longer<br>
> "int"s (because they were now being understood by clang :-)). The flip<br>
> side of that is of course that the sip_generator can now correctly<br>
> handle quite a bit more syntax! I'll be adding some unit tests for<br>
> that stuff too.<br>
><br>
> You can see the WIP at<br>
> <a href="https://github.com/ShaheedHaque/extra-cmake-modules/tree/KDE_master_rebase_with_module_separation" rel="noreferrer" target="_blank">https://github.com/<wbr>ShaheedHaque/extra-cmake-<wbr>modules/tree/KDE_master_<wbr>rebase_with_module_separation</a><br>
> (but please don't attempt to merge anything yet).<br>
><br>
> More anon.<br>
><br>
> Thanks, Shaheed<br>
><br>
> [1] To all-but-the-tip of master, that is your<br>
> 3e6eb0562e5fd3831d66db4720c67c<wbr>c950b3536c.<br>
> [2] As well as incorporating your feedback about the second<br>
> declaration thing and so on.<br>
><br>
> On 22 January 2017 at 15:04, Shaheed Haque <<a href="mailto:srhaque@theiet.org">srhaque@theiet.org</a>> wrote:<br>
>> I see the point. I'll take another look...<br>
>><br>
>> On 22 January 2017 at 12:25, Stephen Kelly <<a href="mailto:steveire@gmail.com">steveire@gmail.com</a>> wrote:<br>
>>><br>
>>> Shaheed Haque wrote:<br>
>>><br>
>>> > Hi Steve,<br>
>>> ><br>
>>> > I've tested that this change does not appear to break things. Full<br>
>>> > details, including the testing, in<br>
>>> > <a href="https://git.reviewboard.kde.org/r/129763/" rel="noreferrer" target="_blank">https://git.reviewboard.kde.<wbr>org/r/129763/</a>. Kindly review.<br>
>>><br>
>>> Hi Shaheed,<br>
>>><br>
>>> Source files in kcoreaddons are in<br>
>>><br>
>>>  util/kuser.h<br>
>>><br>
>>> for example and then get installed to<br>
>>><br>
>>>  include/KF5/KCoreAddons/kuser.<wbr>h<br>
>>><br>
>>> for example.<br>
>>><br>
>>> Currently the buildsystem adds both<br>
>>><br>
>>>  include/KF5/KCoreAddons<br>
>>><br>
>>> and<br>
>>><br>
>>>  include/KF5<br>
>>><br>
>>> to the include directories when a consumer users KCoreAddons. That means<br>
>>> that a consumer can write either<br>
>>><br>
>>>  #include <kuser.h><br>
>>><br>
>>>  (because of the include/KF5/KCoreAddons)<br>
>>><br>
>>> or<br>
>>><br>
>>>  #include <KCoreAddons/kuser.h><br>
>>><br>
>>>  (because of the include/KF5)<br>
>>><br>
>>><br>
>>> I consider the former legacy and I think we should change it to require<br>
>>> the<br>
>>> module name for KF6.<br>
>>><br>
>>> So, the Sip file needs to know to process<br>
>>><br>
>>>  util/kuser.h<br>
>>><br>
>>> but it needs to generate<br>
>>><br>
>>>  %TypeHeaderCode<br>
>>>  #include <KCoreAddons/kuser.h><br>
>>>  %End<br>
>>><br>
>>><br>
>>> That is why the python interface requires specifying both.<br>
>>><br>
>>> It is true that currently the CMake module/function only requires one of<br>
>>> those, and it is the cmake function that makes the assumption that using<br>
>>> basename is all that is needed. That is only because the CMake API for<br>
>>> mappings or pairs like that is inconvenient. I do plan on implementing<br>
>>> it though eventually. Other libraries (outside of KF5) do require<br>
>>> specifying the directory name like that in includes, so it is a<br>
>>> requirement anyway.<br>
>>><br>
>>> The assumption that the two are related by exactly a call to basename<br>
>>> should<br>
>>> not be in the python code/interface.<br>
>>><br>
>>> Does that make sense?<br>
>>><br>
>>> Thanks,<br>
>>><br>
>>> Steve.<br>
>><br>
>><br>
</div></div></blockquote></div><br></div>