On Fri, Sep 28, 2018 at 4:13 PM Andrew Haley <a...@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
