krakjoe / ustring

UnicodeString for PHP7
Other
64 stars 7 forks source link

Fix build with master #13

Closed weltling closed 10 years ago

weltling commented 10 years ago

So far tests past on master/windows.

weltling commented 10 years ago

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

datibbaw commented 10 years ago

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

weltling commented 10 years ago

refixed again, please take a look

krakjoe commented 10 years ago

Thanks :)

pierrejoye commented 10 years ago

Also this part is an absolute no-go:

/*

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 https://github.com/krakjoe/ustring/pull/13#issuecomment-53536309.

Pierre

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

weltling commented 10 years ago

@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 commented 10 years ago

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 commented 10 years ago

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 https://github.com/krakjoe/ustring/pull/13#issuecomment-53544751.

Pierre

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

krakjoe commented 10 years ago

@weltling could you bring config.w32 in line please

weltling commented 10 years ago

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