ticket:12766 Escaping source code

ticket:12766 Escaping source code

[andy.heninger, 2017-03-20 20:17:56]

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/platform.h
File source/common/unicode/platform.h (right):

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/platfor...
source/common/unicode/platform.h:489: #if (U_PLATFORM == U_PF_AIX) &&
defined(__cplusplus) &&(U_CPLUSPLUS_VERSION < 11)
I think we should simply yank anything related to c++ versions < 11, and let it
fail hard.

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/umachine.h
File source/common/unicode/umachine.h (right):

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/umachin...
source/common/unicode/umachine.h:298: #if (U_PLATFORM == U_PF_AIX) &&
defined(__cplusplus) &&(U_CPLUSPLUS_VERSION < 11)
Remove the U_CPLUSPLUS_VERSION < 11

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/unistr.h
File source/common/unicode/unistr.h (right):

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/unistr....
source/common/unicode/unistr.h:125: # define UNICODE_STRING(cs, _length)
icu::UnicodeString(TRUE, (const char16_t*)u ## cs, _length)
How does this fail without this #ifdef? On AIX, wouldn't the type of a u"string"
be compatible with whatever <uchar.h> typedefs char16_t to?

https://codereview.appspot.com/314660043/diff/1/source/configure
File source/configure (right):

https://codereview.appspot.com/314660043/diff/1/source/configure#newcode8516
source/configure:8516: "tools/escapesrc/Makefile") CONFIG_FILES="$CONFIG_FILES
tools/escapesrc/Makefile" ;;
For Windows, I assume we aren't even trying to build escapesrc, since we don't
need it there. Hence, nothing changed in Windows project files.

https://codereview.appspot.com/314660043/diff/1/source/test/intltest/rbbitst.cpp
File source/test/intltest/rbbitst.cpp (right):

https://codereview.appspot.com/314660043/diff/1/source/test/intltest/rbbitst....
source/test/intltest/rbbitst.cpp:1696: #if (U_CPLUSPLUS_VERSION >= 11)
Aren't we requiring C++11? #ifdef-ing things like this is worse than not using
the feature at all.

[srl295, 2017-03-20 20:30:34]

thanks for the reviews

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/platform.h
File source/common/unicode/platform.h (right):

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/platfor...
source/common/unicode/platform.h:489: #if (U_PLATFORM == U_PF_AIX) &&
defined(__cplusplus) &&(U_CPLUSPLUS_VERSION < 11)
On 2017/03/20 20:17:55, andy.heninger wrote:
> I think we should simply yank anything related to c++ versions < 11, and let
it
> fail hard.

The point here is to try to make this code  go away once the compiler actually
claims to support c++11. Once it's not needed, I'd yank this block. I'm trying
to avoid simply "#ifdef AIX … #endif". Right now this is needed even with
so-called c++11 mode.

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/umachine.h
File source/common/unicode/umachine.h (right):

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/umachin...
source/common/unicode/umachine.h:298: #if (U_PLATFORM == U_PF_AIX) &&
defined(__cplusplus) &&(U_CPLUSPLUS_VERSION < 11)
On 2017/03/20 20:17:55, andy.heninger wrote:
> Remove the U_CPLUSPLUS_VERSION < 11

as before, this is here as a future proof.

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/unistr.h
File source/common/unicode/unistr.h (right):

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/unistr....
source/common/unicode/unistr.h:125: # define UNICODE_STRING(cs, _length)
icu::UnicodeString(TRUE, (const char16_t*)u ## cs, _length)
On 2017/03/20 20:17:55, andy.heninger wrote:
> How does this fail without this #ifdef? On AIX, wouldn't the type of a
u"string"
> be compatible with whatever <uchar.h> typedefs char16_t to?

No, it ends up being an incompatible type. See  the UnicodeString c'tors.

https://codereview.appspot.com/314660043/diff/1/source/configure
File source/configure (right):

https://codereview.appspot.com/314660043/diff/1/source/configure#newcode8516
source/configure:8516: "tools/escapesrc/Makefile") CONFIG_FILES="$CONFIG_FILES
tools/escapesrc/Makefile" ;;
On 2017/03/20 20:17:56, andy.heninger wrote:
> For Windows, I assume we aren't even trying to build escapesrc, since we don't
> need it there. Hence, nothing changed in Windows project files.

Right now I'm not building it on Windows, no. I'm building it on other platforms
that don't "need it" just to keep the code building… and if we ever wanted a
"make dist" variant that escapes before building the tarball.

https://codereview.appspot.com/314660043/diff/1/source/test/intltest/rbbitst.cpp
File source/test/intltest/rbbitst.cpp (right):

https://codereview.appspot.com/314660043/diff/1/source/test/intltest/rbbitst....
source/test/intltest/rbbitst.cpp:1685: u"\u200B\u0020}",   // Line 5198
We discussed these, of course. I could potentially make the unescaper fix this
case as well.

https://codereview.appspot.com/314660043/diff/1/source/test/intltest/rbbitst....
source/test/intltest/rbbitst.cpp:1696: #if (U_CPLUSPLUS_VERSION >= 11)
On 2017/03/20 20:17:56, andy.heninger wrote:
> Aren't we requiring C++11? #ifdef-ing things like this is worse than not using
> the feature at all.

Arguably, yes. If this needs an #ifdef AIX (I think this code works on z/OS) we
can do that. The compiler version on our public build farm needed this changed
so.

[andy.heninger, 2017-03-20 21:03:55]

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/unistr.h
File source/common/unicode/unistr.h (right):

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/unistr....
source/common/unicode/unistr.h:125: # define UNICODE_STRING(cs, _length)
icu::UnicodeString(TRUE, (const char16_t*)u ## cs, _length)
On 2017/03/20 20:30:34, srl295 wrote:
> On 2017/03/20 20:17:55, andy.heninger wrote:
> > How does this fail without this #ifdef? On AIX, wouldn't the type of a
> u"string"
> > be compatible with whatever <uchar.h> typedefs char16_t to?
> 
> No, it ends up being an incompatible type. See  the UnicodeString c'tors.

Something's going on that I don't understand. I wonder if xlC++ has the distinct
char16_t type hidden away somewhere, is using it for u"character literals", but
we're getting a different definition from including <uchar.h>


Can you check if _Char16_t works as a type? It's supposed to be another name for
char16_t. http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n2018.html

Another experiment would be to try including <cuchar> instead of <uchar.h>

[srl295, 2017-03-20 21:43:43]

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/unistr.h
File source/common/unicode/unistr.h (right):

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/unistr....
source/common/unicode/unistr.h:125: # define UNICODE_STRING(cs, _length)
icu::UnicodeString(TRUE, (const char16_t*)u ## cs, _length)
On 2017/03/20 21:03:55, andy.heninger wrote:
> On 2017/03/20 20:30:34, srl295 wrote:
> > On 2017/03/20 20:17:55, andy.heninger wrote:
> > > How does this fail without this #ifdef? On AIX, wouldn't the type of a
> > u"string"
> > > be compatible with whatever <uchar.h> typedefs char16_t to?
> > 
> > No, it ends up being an incompatible type. See  the UnicodeString c'tors.
> 
> Something's going on that I don't understand. I wonder if xlC++ has the
distinct
> char16_t type hidden away somewhere, is using it for u"character literals",
but
> we're getting a different definition from including <uchar.h>
> 
> 
> Can you check if _Char16_t works as a type? It's supposed to be another name
for
> char16_t. http://www.open-std.org/jtc1/sc22/wg21/docs/papers/2006/n2018.html
> 
> Another experiment would be to try including <cuchar> instead of <uchar.h>

without this particular #if, the signature this macro generates I think is
"unsigned char, const uint16_t*, … " so I think it has to do with the resolution
of the const wrapper. I can try these #includes.

[markus.icu, 2017-03-22 16:43:49]

https://codereview.appspot.com/314660043/diff/1/source/Doxyfile.in
File source/Doxyfile.in (right):

https://codereview.appspot.com/314660043/diff/1/source/Doxyfile.in#newcode197
source/Doxyfile.in:197: PREDEFINED             = U_EXPORT2= U_STABLE= U_DRAFT=
U_INTERNAL= U_SYSTEM= U_DEPRECATED= U_OBSOLETE= U_CALLCONV= U_CDECL_BEGIN=
U_CDECL_END=  U_NO_THROW=\ "U_NAMESPACE_BEGIN=namespace icu{"
"U_NAMESPACE_END=}" U_SHOW_CPLUSPLUS_API=1 U_DEFINE_LOCAL_OPEN_POINTER()=
U_IN_DOXYGEN=1 U_OVERRIDE= U_FINAL= UCONFIG_ENABLE_PLUGINS=1
U_CHAR16_IS_TYPEDEF=0 U_CPLUSPLUS_VERSION=11
We should not hardcode the C++ version. We will want to support some C++14/17
features at some point.

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/unistr.h
File source/common/unicode/unistr.h (right):

https://codereview.appspot.com/314660043/diff/1/source/common/unicode/unistr....
source/common/unicode/unistr.h:125: # define UNICODE_STRING(cs, _length)
icu::UnicodeString(TRUE, (const char16_t*)u ## cs, _length)
This is very weird: It looks like char16_t==uint16_t but != type of u"". Makes
no sense, but it looks like you would need the cast for this case.

We could in principle add AIX overloads for whatever type u"" has, but that
would become counterproductive as soon as the AIX compiler conforms to the
standard.

[srl295, 2017-03-22 16:55:03]

https://codereview.appspot.com/314660043/diff/1/source/Doxyfile.in
File source/Doxyfile.in (right):

https://codereview.appspot.com/314660043/diff/1/source/Doxyfile.in#newcode197
source/Doxyfile.in:197: PREDEFINED             = U_EXPORT2= U_STABLE= U_DRAFT=
U_INTERNAL= U_SYSTEM= U_DEPRECATED= U_OBSOLETE= U_CALLCONV= U_CDECL_BEGIN=
U_CDECL_END=  U_NO_THROW=\ "U_NAMESPACE_BEGIN=namespace icu{"
"U_NAMESPACE_END=}" U_SHOW_CPLUSPLUS_API=1 U_DEFINE_LOCAL_OPEN_POINTER()=
U_IN_DOXYGEN=1 U_OVERRIDE= U_FINAL= UCONFIG_ENABLE_PLUGINS=1
U_CHAR16_IS_TYPEDEF=0 U_CPLUSPLUS_VERSION=11
On 2017/03/22 16:43:49, markus.icu wrote:
> We should not hardcode the C++ version. We will want to support some C++14/17
> features at some point.

I'd think we would bump the number here then.  This controls what doxygen's view
of the source is.