Review Request 127265: Fix windows build of Ki18n

[Kåre Särs]

> On March 3, 2016, 4:51 p.m., Aleix Pol Gonzalez wrote:
> > src/kcatalog.cpp, line 150
> > <https://git.reviewboard.kde.org/r/127265/diff/1/?file=447929#file447929line150>
> >
> >     Wouldn't it be the same to define it like this?
> >     ```
> >     #if defined(__USE_GNU_GETTEXT)
> >     extern "C" int Q_DECL_IMPORT _nl_msg_cat_cntr;
> >     #endif
> >     ```
> >     
> >     This way we wouldn't have to make special cases for each platform.
> >     
> >     I just tried and works here (on Linux+Clang).
> 
> Andre Heinecke wrote:
>     Ah nice, did not know / remembered this macro. I think in that case we can also drop the extern int declaration inside the function as we already declare it now in a platform independent manner as an external variable.
>     
>     I've tested this again with mingw and GNU/Linux GCC and it still compiled.
> 
> Andreas Cord-Landwehr wrote:
>     Just tested, also works fine under Android.

+1

I think the bug in the last review change was that the extern declaration was inside the function. I'm not sure if the #ifdef was a real problem as you probably need the __declspec() also with MinGW.

This solution is however the best so far :)


- Kåre


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


On March 3, 2016, 5:49 p.m., Andre Heinecke wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127265/
> -----------------------------------------------------------
> 
> (Updated March 3, 2016, 5:49 p.m.)
> 
> 
> Review request for KDE Frameworks, Andreas Cord-Landwehr and Kåre Särs.
> 
> 
> Repository: ki18n
> 
> 
> Description
> -------
> 
> In review Request 127048 (Restrict _nl_msg_cat_cntr use to GNU gettext implentations.) Unrelated Windows changes were requested and then introduced which in my opinion were wrong (and in the opinion of my compiler ;-) ).
> 
> You can't just do an extern "C" dllimport declaration inside a function body:
> 
> src/kcatalog.cpp:190:16: error: expected unqualified-id before string constant
>          extern "C" int __declspec(dllimport) _nl_msg_cat_cntr;
> 
> 
> Also that patch changed the Logic from an MSVC specific define to general Q_OS_WIN. This appears to be wrong, too. While I'm only testing with MinGW I'm pretty sure that the ifndef _MSC_VER was there for a reason.
> 
> This patch reverts the declaration move and logic change while keeping the __USE_GNU_GETTEXT guard.
> 
> 
> Diffs
> -----
> 
>   src/kcatalog.cpp 083443d 
> 
> Diff: https://git.reviewboard.kde.org/r/127265/diff/
> 
> 
> Testing
> -------
> 
> Compiled with mingw and __USE_GNU_GETTEXT for Windows. Also compiled for GNU/Linux.
> 
> 
> Thanks,
> 
> Andre Heinecke
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20160303/5cd81234/attachment.html>