Re: [Tarantool-patches] [PATCH] Add missed icu symbols

[Alexandr Barulev <huston.mavr@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 2666 bytes --]

Hello, thank you for the review!

I took into account the comments and soon i will send new version
of the patch.

About additional macroses like EXPORT_ICU and others, as Timur
mentioned, seems there is no need to create them, because as I
noted in commentary at src/exports_icu.h - unicode/urename.h
file do this work (maps exported symbols to symbols with version
suffix, if library is build with option --enable-renaming). Also we
can't rely on U_ICU_ENTRY_POINT_RENAME macros, because old versions
of libicu don't contain this macros and unicode/urename.h has a
simple symbols mapping:
#define symbol symbol_suffix
So, I think, include <unicode/urename.h> is enough.

Regards,
Alexandr

вт, 22 сент. 2020 г. в 00:05, Timur Safin <tsafin@tarantool.org>:

> : From: Vladislav Shpilevoy <v.shpilevoy@tarantool.org>
> : Subject: Re: [PATCH] Add missed icu symbols
> :
> ...
> : > +EXPORT(u_strlen)
> : > +EXPORT(u_uastrcpy)
> : > +EXPORT(u_austrcpy)
> : > +EXPORT(u_errorName)
> : > +EXPORT(u_getVersion)
> : > +EXPORT(udat_open)
> : > +EXPORT(udat_setLenient)
> : > +EXPORT(udat_close)
> : > +EXPORT(udat_parseCalendar)
> : > +#if U_ICU_VERSION_MAJOR_NUM >= 55
> : > +    EXPORT(udat_formatCalendar)
> : > +#endif
> : > +EXPORT(ucal_open)
> : > +EXPORT(ucal_close)
> : > +EXPORT(ucal_get)
> : > +EXPORT(ucal_set)
> : > +EXPORT(ucal_add)
> : > +EXPORT(ucal_clear)
> : > +EXPORT(ucal_clearField)
> : > +EXPORT(ucal_getMillis)
> : > +EXPORT(ucal_setMillis)
> : > +EXPORT(ucal_getAttribute)
> : > +EXPORT(ucal_setAttribute)
> : > +#if U_ICU_VERSION_MAJOR_NUM >= 51
> : > +    EXPORT(ucal_getTimeZoneID)
> : > +#endif
> : > +EXPORT(ucal_setTimeZone)
> : > +EXPORT(ucal_getNow)
> :
> : 3. Lets better store everything in exports.h. You can add a new
> : section in this file separated by a comment, that these symbols are
> : from ICU, to not scatter them among other exports. Also please sort
> : the exports in the alphabetical order, as it is asked in exports.h
> : comment.
>
> Agreed with Vlad here - keeping all inside of single exports.h would make
> this code much, much easier to maintain. I'd recommend you to return back
> to the original idea with using special EXPORT_ICU(name) macros which
> would
> expand to EXPORT(U_ICU_ENTRY_POINT_RENAME(name)) _iff_ not
> U_DISABLE_RENAMING
> and to EXPORT(name) otherwise.
>
> And for +#if U_ICU_VERSION_MAJOR_NUM >= 51 I'd suggest (but not insist) to
> create yet another wrapper EXPORT_ICU_51UP(name). This would make it looks
> very consistent with others. Though with 1 usage case it might be overkill.
>
> Regards,
> Timur
>
>

[-- Attachment #2: Type: text/html, Size: 3270 bytes --]