Avoid allocating memory using strdup() and then releasing it using
operator delete. strdup()-allocated memory should be released using
free(), not delete. But in these specific cases, there really was
never any good reason to duplicate the C-style strings in the first
place. Instead, we can safely pass those NUL-terminated char pointers
directly in calls to JNI's NewStringUTF(). NewStringUTF() does not
take ownership of its argument, but rather clones that string data
internally before returning. So using strdup() and delete was just
unnecessary memory churn.
In cases where we need to format, concatenate, or construct new
strings, don't use sprintf() into variable-sized, stack-allocated
arrays. Variable-sized arrays are not portable, and in particular are
rejected by Microsoft's Visual Studio 2017 C++ compiler. Instead, do
our string formatting and manipulations using std::ostringstream
and/or std::string. We just need to be a bit careful about the
lifetimes and ownership responsibilities of allocated data. In
brief, (1) ostringstream::str() returns a temporary string instance
that expires at the end of the enclosing statement, independent of the
lifetime of the ostringstream instance; while (2) string::c_str()
returns an pointer to internal data that remains valid as long as the
string on which it was called is valid and unmodified.
Eclipse's automated code clean-up tool did most of the heavy lifting
here: it specifically has a clean-up option for converting functional
interfaces to lambdas. I merely had to revert the automated changes
for a single enumeration class for which it produced invalid results,
and for a few test inputs that apparently aren't set up to be compiled
with Java 8.
Previously FilterIterator was very permissive regarding the type
relationships between the original iterator, the filtered iterator,
and the predicate used to prune the former down to the latter. Now we
enforce those relationships more strictly, including proper use of
covariant ("<? extends T>") and contravariant ("<? super T>")
polymorphic type parameters where appropriate.
This lets us get rid of seven suppressed warnings about generic types
and/or unchecked conversions. It also moves us toward being able to
use modern Java features like lambdas and streams more easily.
Each of these required careful consideration of what the original
developer *intended* as distinguished from what the developer's code
actually *does*. I believe I got each one right, and WALA's
regression tests agree. A second opinion by a core WALA developer
would be welcome, though.
We already have plenty of examples of Serializable classes with this
field, and the vast majority of those fields have generated IDs rather
than "1L". From this I infer that using proper serialVersionUID
fields is considered appropriate WALA coding style.
In general, my approach was to try to eliminate each unused parameter
using Eclipse's "Change Method Signature" refactoring. That did not
always succeed: a parameter may be unused in some base class method,
but then be used in subclass's override of that method. In cases
where refactoring to eliminate a parameter failed, I instead annotated
the parameter with '@SuppressWarnings("unused")' to silence the
warning.
Note: this group of changes creates a significant risk of
incompatibility for third-party WALA code. Some removed parameters
change externally-visible APIs. Furthermore, these changes do not
necessarily lead to Java compilation errors. For example, suppose
third-party code subclasses a WALA class or interface, overrides a
method, but does not annotate that method as @Override. Removing a
parameter means that the third-party method no longer overrides. This
can quietly change code behavior without compile-time errors or
warnings. This is exactly why one should use @Override wherever
possible, but we cannot guarantee that third-party WALA users have
done that.
Unnecessary "throws" declarations tend to cascade. If foo() calls
bar() and bar() falsely declares that it might throw IOException, that
often leads a programmer to declare that foo() might throw IOException
as well. Fixing the bar() throws declaration then reveals that we can
fix the foo() throws declaration too. By the time we reach a fixed
point with cleaning these up, we have removed roughly 320 unnecessary
throws declarations.
In a few cases, this cleanup even lets us remove entire "try
... catch" statements where the only thing being caught was an
exception that we now statically know cannot be thrown. Nice!
In Eclipse project configurations, upgrade any future such shenanigans
from warnings to errors. Now that we've fixed this, we don't want it
coming back again.
There is a potential drawback to this change. Conceivably some public
WALA API entry point might have declared that it could throw some
exception merely to reserve the *option* of throwing that exception in
third-party code that subclasses and overrides the API entry point in
question. I have no idea whether this is a significant concern in
practice, though.
I have *not* upgraded this problem to be treated as an error in the
future. Unfortunately Eclipse uses a single configuration setting for
both unnecessary semicolons and also for empty control-flow statements
like `while (p) ;`. I'm not convinced that it's worth rewriting all
instances of the latter into `while (p) { }`. So this is just going
to stay as a warning for now.
Ben Liblit
Julian Dolby
Julian Dolby