Fix build with master #13

weltling · 2014-08-26T17:40:34Z

fixed identifiers, now after the renames in master passed
fixed windows build
fixed config.m4 with possible libstdc++ linkage bug

So far tests past on master/windows.

hikari-no-yume · 2014-08-26T18:43:48Z

api.h

-PHP_USTRING_API void           php_ustring_construct(zval *that, const char *value, zend_size_t len, const char *codepage, zend_size_t clen TSRMLS_DC);
-PHP_USTRING_API zend_size_t    php_ustring_length(zval *that TSRMLS_DC);
+PHP_USTRING_API void           php_ustring_construct(zval *that, const char *value, size_t len, const char *codepage, size_t clen TSRMLS_DC);
+PHP_USTRING_API size_t    php_ustring_length(zval *that TSRMLS_DC);


Indentation borked.

Conflicts: api.h backend/api.h backend/icu.cpp ustring.cpp

weltling · 2014-08-26T19:31:44Z

@TazeTSchnitzel thanks, had to merge with the origin anyway as i was too late for two hours :) Should be fine now.

Ref: issue #13

krakjoe · 2014-08-27T04:38:11Z

ustring.cpp

@@ -22,6 +22,8 @@
 #include "config.h"
 #endif

+#include "unicode/unistr.h"


We can't include this here, it has to be in backend only.

Unfortunately it needs to be here, otherwise it causes linking issues on Windows. IMHO cleaner were even not to include a cpp file, but add it to the compiler separately.

It can be here, only #ifdef'ed
On Aug 27, 2014 6:38 AM, "Joe Watkins" notifications@github.com wrote:

In ustring.cpp:

@@ -22,6 +22,8 @@
#include "config.h"
#endif

+#include "unicode/unistr.h"

We can't include this here, it has to be in backend only.

—
Reply to this email directly or view it on GitHub
https://github.com/krakjoe/ustring/pull/13/files#r16757160.

That's okay for now I guess ... Liz intends to write a native windows api backend, when we have that we can tidy config.m4/w32, and this include I guess ..

datibbaw · 2014-08-27T04:41:47Z

Most of the code fixes proposed here have already been merged into master; I didn't touch the config.* files, though.

krakjoe · 2014-08-27T04:42:15Z

ustring.cpp

@@ -379,7 +381,7 @@ PHP_METHOD(UString, getCodepage) {
 /* {{{ proto void UString::setDefaultCodepage(string codepage) */
 PHP_METHOD(UString, setDefaultCodepage) {
 	char *codepage = NULL;
-	size_t  clen = 0;
+	int  clen = 0;


are we using int for string length still ??

Yes, according to LXR

Well ... I am well and truly confused ... I just opened my eyes ... maybe it's that ...

Yeah, IMHO that is fine, many libraries don't support 64 bit length strings, so even we had size_t for all - there were many places to check for overflows. Anyway, this is what the current status is so it works with master.

Conflicts: api.h backend/api.h backend/icu.cpp ustring.cpp

weltling · 2014-08-27T07:40:26Z

refixed again, please take a look

Fix build with master

krakjoe · 2014-08-27T08:56:21Z

Thanks :)

pierrejoye · 2014-08-27T09:00:07Z

Also this part is an absolute no-go:

/*

Do some magical ifdef'ing maybe ?
*/
#include "backend/icu.cpp"

It just does what you tried to avoid, having backend specific code in
ustring.cpp. A hpp should be created and used. It won't change what will be
linked with the final .so/dll but the .o(bj) for ustring will simply
include icu.o in this case, or better said, icu.cpp will be inlined in
ustring.cpp.

The

+#include "unicode/unistr.h"
If after using the include "icu.hpp" we still have linkers error for
ustring.o(bj), we can keep doing it. But it should be #ifdefed, not
for windows but if ICU is present and used. I suppose it will be part
of the configure options so something along this line could work:

#ifdef ICU_ENABLED

include "unicode/unistr.h"

#endif

But I somehow think that it won't be required anymore as nothing in
ustring.cpp relies on ICU APIs as far as I can see.

On Wed, Aug 27, 2014 at 9:40 AM, Anatol Belski notifications@github.com
wrote:

refixed again, please take a look

—
Reply to this email directly or view it on GitHub
#13 (comment).

Pierre

@pierrejoye | http://www.libgd.org

weltling · 2014-08-27T09:13:04Z

@krakjoe thanks for merging. I'd also say including a cpp file is something one shouldn't do. That might work or not with cpp, so better use the common way with separate objects and export headers.

krakjoe · 2014-08-27T09:14:07Z

the include there is temporary, when more than one backend exists, configure will handle them and will handle the build too, so it will be built normally ... kinda difficult/pointless to test with one backend ...

pierrejoye · 2014-08-27T09:43:32Z

it can be temporary and correct, but including .cpp is a bad practice (or
.c for what matters) and introduces all kind of issues.

On Wed, Aug 27, 2014 at 11:14 AM, Joe Watkins notifications@github.com
wrote:

the include there is temporary, when more than one backend exists,
configure will handle them and will handle the build too, so it will be
built normally ...

—
Reply to this email directly or view it on GitHub
#13 (comment).

Pierre

@pierrejoye | http://www.libgd.org

krakjoe · 2014-08-27T10:13:31Z

@weltling could you bring config.w32 in line please

weltling · 2014-08-27T10:29:48Z

@krakjoe, ah, you split it ... yep, gonna do that )

weltling added 4 commits August 26, 2014 18:21

first big shot on fixing the master build

a6d14a3

more fixes to configs

000f38e

correct the libs/headers check conds

f40a7a8

missing header

24160c6

hikari-no-yume reviewed Aug 26, 2014
View reviewed changes

weltling added 2 commits August 26, 2014 21:22

Merge https://github.com/krakjoe/ustring

d40f76d

Conflicts: api.h backend/api.h backend/icu.cpp ustring.cpp

remove unneeded TSRMS_CC

496c1d1

weltling referenced this pull request Aug 26, 2014

update for api changes

cb1cff4

datibbaw added a commit that referenced this pull request Aug 27, 2014

Fixed ZTS build

f43805a

Ref: issue #13

krakjoe reviewed Aug 27, 2014
View reviewed changes

weltling added 2 commits August 27, 2014 09:37

Merge https://github.com/krakjoe/ustring

c0b9155

Conflicts: api.h backend/api.h backend/icu.cpp ustring.cpp

fix header and TS

40555c2

krakjoe added a commit that referenced this pull request Aug 27, 2014

Merge pull request #13 from weltling/master

1627ee5

Fix build with master

krakjoe merged commit 1627ee5 into krakjoe:master Aug 27, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix build with master #13

Fix build with master #13

weltling commented Aug 26, 2014

hikari-no-yume Aug 26, 2014

weltling commented Aug 26, 2014

krakjoe Aug 27, 2014

weltling Aug 27, 2014

pierrejoye Aug 27, 2014

krakjoe Aug 27, 2014

datibbaw commented Aug 27, 2014

krakjoe Aug 27, 2014

datibbaw Aug 27, 2014

krakjoe Aug 27, 2014

weltling Aug 27, 2014

weltling commented Aug 27, 2014

krakjoe commented Aug 27, 2014

pierrejoye commented Aug 27, 2014

weltling commented Aug 27, 2014

krakjoe commented Aug 27, 2014

pierrejoye commented Aug 27, 2014

krakjoe commented Aug 27, 2014

weltling commented Aug 27, 2014

Fix build with master #13

Fix build with master #13

Conversation

weltling commented Aug 26, 2014

Choose a reason for hiding this comment

weltling commented Aug 26, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

datibbaw commented Aug 27, 2014

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

weltling commented Aug 27, 2014

krakjoe commented Aug 27, 2014

pierrejoye commented Aug 27, 2014

include "unicode/unistr.h"

weltling commented Aug 27, 2014

krakjoe commented Aug 27, 2014

pierrejoye commented Aug 27, 2014

krakjoe commented Aug 27, 2014

weltling commented Aug 27, 2014