harmony-dev mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "Andrew Zhang" <zhanghuang...@gmail.com>
Subject Re: [classlib][luni] Tweaked native method name in file
Date Wed, 11 Oct 2006 13:30:37 GMT
On 10/11/06, Spark Shen <smallsmallorgan@gmail.com> wrote:

> Hi All:
> After skimming through the source code of java.io.File, I found two
> tweaked method names:
>
> private native boolean isReadOnlyImpl(byte[] filePath);
> private native boolean isWriteOnlyImpl(byte[] filePath);
>
> Take isWriteOnlyImpl as an example,
>
> canRead ->
> exists && !Java_java_io_File_isWriteOnlyImpl() ->       => this method
> is in native\luni\shared\file.c
> getPlatformIsWriteOnly()  => this method is in
> native\luni\linux\helpers.c, and the follow is the code snippet
>
> if (buffer.st_uid == geteuid ())
>    return (buffer.st_mode & S_IRUSR) == 0;
> else if (buffer.st_gid == getegid ())
>    return (buffer.st_mode & S_IRGRP) == 0;
>
> return (buffer.st_mode & S_IROTH) == 0;
>
> The name isWriteOnlyImpl is confusing, negate the return value of
> isWriteOnlyImpl and uses it in canRead is confusing,
> in native method getPlatformIsWriteOnly(), prob S_IRXXX bit is also
> confusing and can not express the meaning writeOnly. Correct me if I am
> wrong.
> Since isWriteOnlyImpl is only utilized by canRead, I suggest the
> following:
>
> change isWriteOnlyImpl to isReadableImpl, getPlatformIsWriteOnly to
> getPlatformIsReadable and do the negation in getPlatformIsReadable:


Agree. But I don't think it's "negation". you may simplify the code as:

if (buffer.st_uid == geteuid ())
   return buffer.st_mode & S_IRUSR;
...



> canRead ->
> exists && Java_java_io_File_isReadOnlyImpl() -> getPlatformIsReadable()
>
> if (buffer.st_uid == geteuid ())
>    return (buffer.st_mode & S_IRUSR) != 0;
> else if (buffer.st_gid == getegid ())
>    return (buffer.st_mode & S_IRGRP) != 0;
>
> return (buffer.st_mode & S_IROTH) != 0;
>
> This way, code is more readable and the method name
> getPlatformIsReadable semantically matches it implemantation.
> (The original name getPlatformIsWriteOnly seems not consistent with its
> implmentation, correct me if I am wrong).


Agree. I think we'd better fix it, although it's only an internal method.


> If no one objects, I want to supply a patch to do the enhancement.
>
> Best regards
>
> --
> Spark Shen
> China Software Development Lab, IBM
>
>
> ---------------------------------------------------------------------
> Terms of use : http://incubator.apache.org/harmony/mailing.html
> To unsubscribe, e-mail: harmony-dev-unsubscribe@incubator.apache.org
> For additional commands, e-mail: harmony-dev-help@incubator.apache.org
>
>


-- 
Best regards,
Andrew Zhang

Mime
  • Unnamed multipart/alternative (inline, None, 0 bytes)
View raw message