From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-oi1-f196.google.com (mail-oi1-f196.google.com [209.85.167.196]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by dev.tarantool.org (Postfix) with ESMTPS id DA6FF469719 for ; Wed, 23 Sep 2020 01:21:06 +0300 (MSK) Received: by mail-oi1-f196.google.com with SMTP id a3so22823812oib.4 for ; Tue, 22 Sep 2020 15:21:06 -0700 (PDT) MIME-Version: 1.0 References: <20200921195106.96821-1-huston.mavr@gmail.com> <0a4d01d6905a$ed4c1110$c7e43330$@tarantool.org> In-Reply-To: <0a4d01d6905a$ed4c1110$c7e43330$@tarantool.org> From: Alexandr Barulev Date: Wed, 23 Sep 2020 01:20:53 +0300 Message-ID: Content-Type: multipart/alternative; boundary="000000000000fad2ac05afee619d" Subject: Re: [Tarantool-patches] [PATCH] Add missed icu symbols List-Id: Tarantool development patches List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , To: Timur Safin Cc: Alexander Turenko , tarantool-patches@dev.tarantool.org, Vladislav Shpilevoy , Yaroslav Dynnikov --000000000000fad2ac05afee619d Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable 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 is enough. Regards, Alexandr =D0=B2=D1=82, 22 =D1=81=D0=B5=D0=BD=D1=82. 2020 =D0=B3. =D0=B2 00:05, Timur= Safin : > : From: Vladislav Shpilevoy > : 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 >=3D 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 >=3D 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 >=3D 51 I'd suggest (but not insist)= to > create yet another wrapper EXPORT_ICU_51UP(name). This would make it look= s > very consistent with others. Though with 1 usage case it might be overkil= l. > > Regards, > Timur > > --000000000000fad2ac05afee619d Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hello, thank you for the review!

I took into accoun= t the comments and soon i will send new version
of the patch.

Abo= ut additional macroses like EXPORT_ICU and others, as Timur
mentioned, s= eems there is no need to create them, because as I
noted in commentary a= t 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 symbo= l_suffix
So, I think, include <unicode/urename.h> is enough.
Regards,
Alexandr

=D0=B2=D1=82, 22 =D1=81=D0=B5=D0=BD=D1=82. 2020 =D0= =B3. =D0=B2 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 >=3D 55
: > +=C2=A0 =C2=A0 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 >=3D 51
: > +=C2=A0 =C2=A0 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 <= br> this code much, much easier to maintain. I'd recommend you to return ba= ck
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_RENAMI= NG
and to EXPORT(name) otherwise.

And for +#if U_ICU_VERSION_MAJOR_NUM >=3D 51 I'd suggest (but not in= sist) 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

--000000000000fad2ac05afee619d--