[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR(M): JDK-8041498 Remove npt library
From:       Dmitry Samersoff <dmitry.samersoff () oracle ! com>
Date:       2014-04-24 20:29:33
Message-ID: 5359742D.1070603 () oracle ! com
[Download RAW message or body]

Staffan,

After some "whitebox" testing, I decided to change conversion code to
have consistent error handling and consistent behavior of UNIX and
windows parts.

http://cr.openjdk.java.net/~dsamersoff/JDK-8041498/webrev.02/

(besides fixed nits only utf_util.c is changed)

Changes details:

1. Original code try to convert to/from ANSI_X3.4-1968 (default value
returned by nl_langinfo) if no locale set and it's not correct.
  Modified code check result of setlocale() and don't convert if no
locale is set.

2. Original code leaves output empty or uninitialized in case of
conversion error. Callers never checks for -1 returned on conversion
error so it can cause weird failures.
  Modified code returns copy of original string in case of conversion error.


Also fixed nits below.


On 2014-04-24 13:52, Staffan Larsen wrote:
> src/share/demo/jvmti/hprof/hprof_init.c
> L743-744: Indentation looks wrong

fixed.

> 
> src/share/back/utf_util.h
> L1: Missing copyright header.

fixed.

> 
> L14: UTF_ERROR and UTF_ASSERT are only used in utf_util.c, so they can be defined \
> in that file instead of the header file.

fixed

> 
> src/share/back/utf_util.c
> L297: Wouldn’t it make sense to cache the result of getCodepage() in a static \
> variable so we don’t need to call this code for each UTF conversion?

fixed.

> 
> L492: skeep -> skip
> 
> L492: This TODO should be fixed. I also have the same worry here as for the windows \
> code: that we should cache data so that we don’t run this unnecessarily. 

fixed

> 
> In the original code we did: 
> 
> /* Set the locale from the environment */
> (void)setlocale(LC_ALL, "”);
> 
> but that is missing from the new code. I don’t know if it matters?

It was intentional, but finally after some "whitebox" testing I decided
to rewrite conversion code. See above.

-Dmitry



> 
> 
> Thanks,
> /Staffan
> 
> 
> 
> On 23 apr 2014, at 17:03, Dmitry Samersoff <dmitry.samersoff@oracle.com> wrote:
> 
> > Hi Everybody,
> > 
> > Please review the fix that removes npt library from jdk.
> > 
> > CR link:
> > 
> > https://bugs.openjdk.java.net/browse/JDK-8041498
> > 
> > Webrev:
> > 
> > http://cr.openjdk.java.net/~dsamersoff/JDK-8041498/webrev.01/
> > 
> > 
> > Testing:
> > 
> > nsk.jdwp
> > 
> > I plan to submit a separate aurora run when I get all concerns.
> > 
> > Fix details:
> > 
> > * Today libnpt is a set of UTF related function and only few of them is
> > actually used. The only users of npt is JDWP and hprof. So it's not
> > necessary to keep it as a separate library.
> > 
> > * Relevant code moved from npt to src/share/back/utf_util.c
> > platfrom specific part is under #ifdef _WINDOWS initialization step
> > is removed.
> > 
> > * hprof changed to don't do any conversion.
> > 
> > The only place where hrpof care about encoding is an output
> > filename. Conversion from platform to UTF8 assume that
> > filesystem does support UTF8 but command line doesn't.
> > This assumption is obviously not correct in all cases and
> > therefore handling of non-ascii filenames is incomplete.
> > We should file a separate RFE if we need to handle no-ascii
> > filenames by hprof.
> > 
> > -Dmitry
> > 
> > -- 
> > Dmitry Samersoff
> > Oracle Java development team, Saint Petersburg, Russia
> > * I would love to change the world, but they won't give me the sources.
> 


-- 
Dmitry Samersoff
Oracle Java development team, Saint Petersburg, Russia
* I would love to change the world, but they won't give me the sources.


[prev in list] [next in list] [prev in thread] [next in thread]