RFR: 8211163: UNIX version of Java_java_io_Console_echo does not return a clean boolean

[Volker Simonis]

On Fri, Sep 28, 2018 at 4:13 PM Andrew Haley <aph at redhat.com> wrote:
>
> On 09/28/2018 02:55 PM, Volker Simonis wrote:
> > I agree that the native code should be fixed and I think your patch is correct.
> >
> > Nevertheless I'd vote for changing it to something like:
> >
> > old = (tio.c_lflag & ECHO) == 0 ? JNI_FALSE : JNI_TRUE;
> >
> > It's slightly more verbose, but it makes it more obvious that a JNI
> > function returning a jboolean should only ever return JNI_FALSE or
> > JNI_TRUE. This isn't obvious from your fix - especially not for C
> > programmers who produce such code.
>
> Where is the evidence that a function returning a jboolean should only
> ever return JNI_FALSE or JNI_TRUE? It doesn't say so in the JNI spec,
> only that these values are provided for convenience. They are defined
> to be 0 and 1, like so:
>
> #define JNI_FALSE  0
> #define JNI_TRUE   1
>
> which are *exactly* the same values that the C != operation
> returns. Therefore there is no advantage either in correctness or
> clarity gained by using
>
>   ? JNI_FALSE : JNI_TRUE
>
> It's not even pedantically correct, merely verbose.
>

Yes, but the next developer who adds a function which returns jboolean
and looks at your implementation could easily introduce new,  buggy
code like the one you fixed (because from a C-perspective that makes
no difference). But if he sees that other JNI functions all return
exclusively JNI_TRUE/JNI_FALSE he may be  inclined to do so as well.

> > Also, it would be good to change the bug summary to something more
> > generic like "Fix native library code which does not return a clean
> > boolean" and fix the additional occurrences of this antipattern
> > reported by Matthias in this mail thread:
> > http://mail.openjdk.java.net/pipermail/jdk-dev/2018-September/thread.html#1986
> > I think it would makelittle sense to open a new issue for each of
> > them.
>
> Probably.
>
> --
> Andrew Haley
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671