Re: Review Request 127265: Fix windows build of Ki18n

2016-03-04 Thread Andre Heinecke 

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

(Updated March 4, 2016, 9:09 a.m.)


Status
--

This change has been marked as submitted.


Review request for KDE Frameworks, Andreas Cord-Landwehr and Kåre Särs.


Changes
---

Submitted with commit b91904c55ef1aaeeb60f9e8153dc8a1f00d77fb0 by Andre 
Heinecke to branch master.


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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127265: Fix windows build of Ki18n

2016-03-04 Thread Andre Heinecke 


> On March 3, 2016, 9:35 p.m., Kevin Funk wrote:
> > Ship It!

Oh *facepalm* while pushing I noticed that I had not updated to latest changes 
and Thomas already fixed this issue,..
Anyhow Thomas patch did not use Q_DECL_IMPORT so I'm pushing that part of this 
patch.


- Andre


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


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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127265: Fix windows build of Ki18n

2016-03-03 Thread Kevin Funk 

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


Ship it!




Ship It!

- Kevin Funk


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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127265: Fix windows build of Ki18n

2016-03-03 Thread Kåre Särs 


> On March 3, 2016, 4:51 p.m., Aleix Pol Gonzalez wrote:
> > src/kcatalog.cpp, line 150
> > 
> >
> > 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127265: Fix windows build of Ki18n

2016-03-03 Thread Andreas Cord-Landwehr 


> On März 3, 2016, 4:51 nachm., Aleix Pol Gonzalez wrote:
> > src/kcatalog.cpp, line 150
> > 
> >
> > 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.

Just tested, also works fine under Android.


- Andreas


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


On März 3, 2016, 5:49 nachm., Andre Heinecke wrote:
> 
> ---
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/127265/
> ---
> 
> (Updated März 3, 2016, 5:49 nachm.)
> 
> 
> 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127265: Fix windows build of Ki18n

2016-03-03 Thread Andre Heinecke 


> On March 3, 2016, 4:51 p.m., Aleix Pol Gonzalez wrote:
> > src/kcatalog.cpp, line 150
> > 
> >
> > 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).

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.


- Andre


---
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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127265: Fix windows build of Ki18n

2016-03-03 Thread Andre Heinecke 

---
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.


Changes
---

used Q_DECL_IMPORT for a portable declaration of \_nl_msg_cat_cntr


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 (updated)
-

  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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Re: Review Request 127265: Fix windows build of Ki18n

2016-03-03 Thread Aleix Pol Gonzalez 

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




src/kcatalog.cpp (line 150)


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).


- Aleix Pol Gonzalez


On March 3, 2016, 3:29 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, 3:29 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
> 
>

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel

Review Request 127265: Fix windows build of Ki18n

2016-03-03 Thread Andre Heinecke 

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

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

___
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel