opendev/gerrit
Code Issues Proposed changes

Turn on many more Eclipse warnings, and fix them

Browse Source 
- Warn on empty statements, e.g. "for (;;);". These may be
   typos and are easily replaced by "for (;;) {}" which is more
   explicit.
 - Warn on field hiding. This allows cleanup of many acceptance test
   members, at the cost of a couple of renames and the occasional
   suppression (when the field is in a public nested enum that shadows
   a public constant).
 - Warn on unnecessary casts.
 - Warn on unused declared thrown exceptions. In addition to reducing
   method signature length and number of imports, this also eliminated
   some impossible catch blocks.
 - Warn on missing @Override annotations.
 - Warn on unused parameters. This is likely the most controversial,
   as a few relatively common patterns require unused parameters in a
   way that Eclipse can't ignore. However, it also resulted in cleanup
   of a lot of unnecessary injections and method parameters, so I
   think the cost was worth it.

Change-Id: I7224be8b1c798613a127c88507e8cce400679e5d
This commit is contained in:
Dave Borowitz
2014-10-28 12:09:55 -07:00
parent 2e82f2f8a2
commit 8b42ec5bd5
305 changed files with 932 additions and 699 deletions
Download Patch File Download Diff File Expand all files Collapse all files

5
gerrit-server/src/main/java/com/google/gerrit/server/plugins/JarPluginProvider.java
View File

@@ -85,7 +85,7 @@ public class JarPluginProvider implements ServerPluginProvider {
File tmp = asTemp(in, tempNameFor(name), extension, tmpDir);
return loadJarPlugin(name, srcFile, snapshot, tmp, description);
}
} catch (IOException | ClassNotFoundException e) {
} catch (IOException e) {
throw new InvalidPluginException("Cannot load Jar plugin " + srcFile, e);
}
}
@@ -119,8 +119,7 @@ public class JarPluginProvider implements ServerPluginProvider {
private ServerPlugin loadJarPlugin(String name, File srcJar,
FileSnapshot snapshot, File tmp, PluginDescription description)
throws IOException, InvalidPluginException, MalformedURLException,
ClassNotFoundException {
throws IOException, InvalidPluginException, MalformedURLException {
JarFile jarFile = new JarFile(tmp);
boolean keep = false;
try {

1
gerrit-server/src/main/java/com/google/gerrit/server/plugins/JarScanner.java
View File

@@ -274,6 +274,7 @@ public class JarScanner implements PluginContentScanner {
return Collections.enumeration(Lists.transform(
Collections.list(jarFile.entries()),
new Function<JarEntry, PluginEntry>() {
@Override
public PluginEntry apply(JarEntry jarEntry) {
try {
return resourceOf(jarEntry);

75
gerrit-server/src/main/java/com/google/gerrit/server/plugins/PluginLoader.java
View File

@@ -71,7 +71,7 @@ public class PluginLoader implements LifecycleListener {
static final String PLUGIN_TMP_PREFIX = "plugin_";
static final Logger log = LoggerFactory.getLogger(PluginLoader.class);
public String getPluginName(File srcFile) throws IOException {
public String getPluginName(File srcFile) {
return MoreObjects.firstNonNull(getGerritPluginName(srcFile),
nameOf(srcFile));
}
@@ -546,7 +546,7 @@ public class PluginLoader implements LifecycleListener {
}
private Plugin loadPlugin(String name, File srcPlugin, FileSnapshot snapshot)
throws IOException, ClassNotFoundException, InvalidPluginException {
throws InvalidPluginException {
String pluginName = srcPlugin.getName();
if (isJsPlugin(pluginName)) {
return loadJsPlugin(name, srcPlugin, snapshot);
@@ -624,43 +624,37 @@ public class PluginLoader implements LifecycleListener {
public Multimap<String, File> prunePlugins(File pluginsDir) {
List<File> pluginFiles = scanFilesInPluginsDirectory(pluginsDir);
Multimap<String, File> map;
try {
map = asMultimap(pluginFiles);
for (String plugin : map.keySet()) {
Collection<File> files = map.asMap().get(plugin);
if (files.size() == 1) {
continue;
}
// retrieve enabled plugins
Iterable<File> enabled = filterDisabledPlugins(
files);
// If we have only one (the winner) plugin, nothing to do
if (!Iterables.skip(enabled, 1).iterator().hasNext()) {
continue;
}
File winner = Iterables.getFirst(enabled, null);
assert(winner != null);
// Disable all loser plugins by renaming their file names to
// "file.disabled" and replace the disabled files in the multimap.
Collection<File> elementsToRemove = Lists.newArrayList();
Collection<File> elementsToAdd = Lists.newArrayList();
for (File loser : Iterables.skip(enabled, 1)) {
log.warn(String.format("Plugin <%s> was disabled, because"
+ " another plugin <%s>"
+ " with the same name <%s> already exists",
loser, winner, plugin));
File disabledPlugin = new File(loser + ".disabled");
elementsToAdd.add(disabledPlugin);
elementsToRemove.add(loser);
loser.renameTo(disabledPlugin);
}
Iterables.removeAll(files, elementsToRemove);
Iterables.addAll(files, elementsToAdd);
map = asMultimap(pluginFiles);
for (String plugin : map.keySet()) {
Collection<File> files = map.asMap().get(plugin);
if (files.size() == 1) {
continue;
}
} catch (IOException e) {
log.warn("Cannot prune plugin list",
e.getCause());
return LinkedHashMultimap.create();
// retrieve enabled plugins
Iterable<File> enabled = filterDisabledPlugins(
files);
// If we have only one (the winner) plugin, nothing to do
if (!Iterables.skip(enabled, 1).iterator().hasNext()) {
continue;
}
File winner = Iterables.getFirst(enabled, null);
assert(winner != null);
// Disable all loser plugins by renaming their file names to
// "file.disabled" and replace the disabled files in the multimap.
Collection<File> elementsToRemove = Lists.newArrayList();
Collection<File> elementsToAdd = Lists.newArrayList();
for (File loser : Iterables.skip(enabled, 1)) {
log.warn(String.format("Plugin <%s> was disabled, because"
+ " another plugin <%s>"
+ " with the same name <%s> already exists",
loser, winner, plugin));
File disabledPlugin = new File(loser + ".disabled");
elementsToAdd.add(disabledPlugin);
elementsToRemove.add(loser);
loser.renameTo(disabledPlugin);
}
Iterables.removeAll(files, elementsToRemove);
Iterables.addAll(files, elementsToAdd);
}
return map;
}
@@ -694,7 +688,7 @@ public class PluginLoader implements LifecycleListener {
});
}
public String getGerritPluginName(File srcFile) throws IOException {
public String getGerritPluginName(File srcFile) {
String fileName = srcFile.getName();
if (isJsPlugin(fileName)) {
return fileName.substring(0, fileName.length() - 3);
@@ -705,8 +699,7 @@ public class PluginLoader implements LifecycleListener {
return null;
}
private Multimap<String, File> asMultimap(List<File> plugins)
throws IOException {
private Multimap<String, File> asMultimap(List<File> plugins) {
Multimap<String, File> map = LinkedHashMultimap.create();
for (File srcFile : plugins) {
map.put(getPluginName(srcFile), srcFile);

36
gerrit-server/src/main/java/com/google/gerrit/server/plugins/ServerPlugin.java
View File

@@ -69,7 +69,7 @@ public class ServerPlugin extends Plugin {
private Injector sysInjector;
private Injector sshInjector;
private Injector httpInjector;
private LifecycleManager manager;
private LifecycleManager serverManager;
private List<ReloadableRegistrationHandle<?>> reloadableHandles;
public ServerPlugin(String name,
@@ -140,12 +140,14 @@ public class ServerPlugin extends Plugin {
}
}
@Override
@Nullable
public String getVersion() {
Attributes main = manifest.getMainAttributes();
return main.getValue(Attributes.Name.IMPLEMENTATION_VERSION);
}
@Override
boolean canReload() {
Attributes main = manifest.getMainAttributes();
String v = main.getValue("Gerrit-ReloadMode");
@@ -161,6 +163,7 @@ public class ServerPlugin extends Plugin {
}
}
@Override
void start(PluginGuiceEnvironment env) throws Exception {
RequestContext oldContext = env.enter(this);
try {
@@ -172,7 +175,7 @@ public class ServerPlugin extends Plugin {
private void startPlugin(PluginGuiceEnvironment env) throws Exception {
Injector root = newRootInjector(env);
manager = new LifecycleManager();
serverManager = new LifecycleManager();
AutoRegisterModules auto = null;
if (sysModule == null && sshModule == null && httpModule == null) {
@@ -182,10 +185,10 @@ public class ServerPlugin extends Plugin {
if (sysModule != null) {
sysInjector = root.createChildInjector(root.getInstance(sysModule));
manager.add(sysInjector);
serverManager.add(sysInjector);
} else if (auto != null && auto.sysModule != null) {
sysInjector = root.createChildInjector(auto.sysModule);
manager.add(sysInjector);
serverManager.add(sysInjector);
} else {
sysInjector = root;
}
@@ -198,11 +201,11 @@ public class ServerPlugin extends Plugin {
if (sshModule != null) {
modules.add(sysInjector.getInstance(sshModule));
sshInjector = sysInjector.createChildInjector(modules);
manager.add(sshInjector);
serverManager.add(sshInjector);
} else if (auto != null && auto.sshModule != null) {
modules.add(auto.sshModule);
sshInjector = sysInjector.createChildInjector(modules);
manager.add(sshInjector);
serverManager.add(sshInjector);
}
}
@@ -214,15 +217,15 @@ public class ServerPlugin extends Plugin {
if (httpModule != null) {
modules.add(sysInjector.getInstance(httpModule));
httpInjector = sysInjector.createChildInjector(modules);
manager.add(httpInjector);
serverManager.add(httpInjector);
} else if (auto != null && auto.httpModule != null) {
modules.add(auto.httpModule);
httpInjector = sysInjector.createChildInjector(modules);
manager.add(httpInjector);
serverManager.add(httpInjector);
}
}
manager.start();
serverManager.start();
}
private Injector newRootInjector(final PluginGuiceEnvironment env) {
@@ -266,44 +269,49 @@ public class ServerPlugin extends Plugin {
return Guice.createInjector(modules);
}
@Override
void stop(PluginGuiceEnvironment env) {
if (manager != null) {
if (serverManager != null) {
RequestContext oldContext = env.enter(this);
try {
manager.stop();
serverManager.stop();
} finally {
env.exit(oldContext);
}
manager = null;
serverManager = null;
sysInjector = null;
sshInjector = null;
httpInjector = null;
}
}
@Override
public Injector getSysInjector() {
return sysInjector;
}
@Override
@Nullable
public Injector getSshInjector() {
return sshInjector;
}
@Override
@Nullable
public Injector getHttpInjector() {
return httpInjector;
}
@Override
public void add(RegistrationHandle handle) {
if (manager != null) {
if (serverManager != null) {
if (handle instanceof ReloadableRegistrationHandle) {
if (reloadableHandles == null) {
reloadableHandles = Lists.newArrayList();
}
reloadableHandles.add((ReloadableRegistrationHandle<?>) handle);
}
manager.add(handle);
serverManager.add(handle);
}
}