[Bug 56878] New: Checking whether unsigned int is less than zero is useless

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=56878

            Bug ID: 56878
           Summary: Checking whether unsigned int is less than zero is
                    useless
           Product: Tomcat Native
           Version: 1.1.31
          Hardware: PC
                OS: Mac OS X 10.4
            Status: NEW
          Severity: trivial
          Priority: P2
         Component: Library
          Assignee: dev@tomcat.apache.org
          Reporter: chris@christopherschultz.net

I just saw this when compiling with clang on Mac OS X (partially because clang
shows all warnings and errors in BIG BOLD SUPER HIGHLIGHTED TEXT!):

src/jnilib.c:130:11: warning: comparison of unsigned expression < 0 is always
      false [-Wtautological-compare]
    if (l < 0)
        ~ ^ ~
1 warning generated.

I checked, and the function [jstring tcn_new_stringn(JNIEnv *env, const char
*str, size_t l)] accepts an argument of size_t and then checks to see if it is
less than 0. Since it's unsigned, it will never be negative.

I checked, and this function is not even used in the tcnative library itself.

Looking at similar functions, I can see that tcn_new_stringn actually checks
the Java runtime to see if new references can be created in the local scope and
fails if it can't. The other similar functions do not do such checking, and are
therefore fragile.

I think it makes sense to review these functions to decide what functions to
eliminate (e.g. tcn_new_stringn) and to what extent will the code in tcnative
do diligent sanity checking rather than letting the runtime fail in various
ways.

-- 
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

[Bug 56878] Checking whether unsigned int is less than zero is useless

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=56878

--- Comment #1 from Konstantin Kolinko <kn...@gmail.com> ---
(In reply to Christopher Schultz from comment #0)
> I just saw this when compiling with clang on Mac OS X (partially because
> clang shows all warnings and errors in BIG BOLD SUPER HIGHLIGHTED TEXT!):
> 
> src/jnilib.c:130:11: warning: comparison of unsigned expression < 0 is always
>       false [-Wtautological-compare]
>     if (l < 0)
>         ~ ^ ~
> 1 warning generated.
> 
> I checked, and the function [jstring tcn_new_stringn(JNIEnv *env, const char
> *str, size_t l)] accepts an argument of size_t and then checks to see if it
> is less than 0. Since it's unsigned, it will never be negative.

What header defines size_t ? Is it platform-dependent?

Passing "-1" as a size might be a valid use case (in theory).

BTW, the "len" local variable is cast to (jsize) on one line and to (jint) on
another.

> Looking at similar functions, I can see that tcn_new_stringn actually checks
> the Java runtime to see if new references can be created in the local scope
> and fails if it can't. The other similar functions do not do such checking,
> and are therefore fragile.

The only other function that calls DeleteLocalRef there (tcn_get_string) has
such check. I do not think anything is fragile there.

Documentation for "EnsureLocalCapacity" [1] says that JVM does not enforce the
limit, but allocates references beyond the ensured capacity.

I think the EnsureLocalCapacity call here just makes it to produce a nice
explicit OutOfMemoryError instead of a NULL return value. I think that it
wouldn't become fragile if EnsureLocalCapacity call is omitted, because on the
following lines all function that allocate objects are checked for returning a
NULL value.

BTW, comparing "tcn_new_stringn" and "tcn_new_string" implementations,
there is subtle difference between them:
- the "tcn_new_string" method calls "NewStringUTF" (so it uses "modified" UTF-8
used by Java),
- the "tcn_new_stringn" method copies C chars into a byte array and uses
platform encoding to convert them into Java characters.

What approach is the correct one depends on where the char* data came from.

[1]
http://docs.oracle.com/javase/1.5.0/docs/guide/jni/spec/functions.html#global_local
[2] http://stackoverflow.com/questions/7081853/jni-ensurelocalcapacity-why

-- 
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org

[Bug 56878] Checking whether unsigned int is less than zero is useless

[bu...@apache.org]

https://issues.apache.org/bugzilla/show_bug.cgi?id=56878

--- Comment #2 from Chuck Caldarale <ch...@unisys.com> ---
(In reply to Konstantin Kolinko from comment #1)
> What header defines size_t ? Is it platform-dependent?
> 
> Passing "-1" as a size might be a valid use case (in theory).

No, size_t is defined to be unsigned by the C and C++ standards.  The range of
values supported is platform-dependent, but they are always zero or positive.

-- 
You are receiving this mail because:
You are the assignee for the bug.

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@tomcat.apache.org
For additional commands, e-mail: dev-help@tomcat.apache.org