You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by oreissig <gi...@git.apache.org> on 2016/03/06 16:09:16 UTC
[GitHub] groovy pull request: Some bulk refactorings
GitHub user oreissig opened a pull request:
https://github.com/apache/groovy/pull/282
Some bulk refactorings
I went through the codebase of the root project and did the following refactorings:
- make private methods static when they are plain functions
- add @Deprecated when javadoc says the method is @deprecated
- remove unused code
- avoid synthetic accessors by making private fields/methods package-private
- make private fields final where possible
- fix some inconsistent whitespaces
None of this should change the behaviour in any way.
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/oreissig/groovy minor-refactorings
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/groovy/pull/282.patch
To close this pull request, make a commit to your master/trunk branch
with (at least) the following in the commit message:
This closes #282
----
commit b05c5668c760684d09ac76f7aca2000c7f2703f7
Author: oreissig <or...@gmail.com>
Date: 2016-03-04T21:35:49Z
make private methods static when they are plain functions
commit 3a3cfb01464f6e193c813fd9ab093c21983c8048
Author: oreissig <or...@gmail.com>
Date: 2016-03-04T21:41:36Z
add @Deprecated when javadoc says the method is @deprecated
commit 1295647fdc90d343d11469d36525b4a3f74bc696
Author: oreissig <or...@gmail.com>
Date: 2016-03-04T22:02:01Z
remove unused code
commit e12ce8938bdb3d78a3e89821810d01d2bd9c55c2
Author: oreissig <or...@gmail.com>
Date: 2016-03-04T22:27:30Z
avoid synthetic accessors by making private fields/methods package-private
commit 46ae52b6a7a794d01c89718a8e4aa69eb22d2778
Author: oreissig <or...@gmail.com>
Date: 2016-03-06T12:54:58Z
make private fields final where possible
commit f74c9c5ccac3d7cfe0420104808ee96457d6d122
Author: oreissig <or...@gmail.com>
Date: 2016-03-06T14:49:02Z
fix some inconsistent whitespaces
----
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request: Some bulk refactorings
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/groovy/pull/282
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #282: Some bulk refactorings
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/282#discussion_r107541243
--- Diff: src/main/org/codehaus/groovy/vmplugin/v7/IndyArrayAccess.java ---
@@ -96,29 +96,6 @@ private static MethodHandle buildSetter(Class arrayClass){
return handle;
}
- private static int getLength(Object array) {
- if (array instanceof Object[]) return ((Object[])array).length;
- if (array instanceof boolean[]) return ((boolean[])array).length;
- if (array instanceof byte[]) return ((byte[])array).length;
- if (array instanceof char[]) return ((char[])array).length;
- if (array instanceof short[]) return ((short[])array).length;
- if (array instanceof int[]) return ((int[])array).length;
- if (array instanceof long[]) return ((long[])array).length;
- if (array instanceof float[]) return ((float[])array).length;
- if (array instanceof double[]) return ((double[])array).length;
- return 0;
- }
-
- private static int normalizeIndex(Object array, int i) {
--- End diff --
normalizeIndex is used via MethodHandle in line 39. you cannot remove these
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #282: Some bulk refactorings
Posted by paulk-asert <gi...@git.apache.org>.
Github user paulk-asert commented on a diff in the pull request:
https://github.com/apache/groovy/pull/282#discussion_r107062503
--- Diff: src/main/groovy/lang/GroovyCodeSource.java ---
@@ -164,7 +164,7 @@ public GroovyCodeSource(URI uri) throws IOException {
this(uri.toURL());
}
- public GroovyCodeSource(URL url) throws IOException {
--- End diff --
I only just spotted this now. While "unused" in the sense that this method didn't throw this exception anymore (I presume it did in the past), removing the exception breaks contract/source code compatibility. :-( I know it is a result of how checked exceptions are handled on the JVM but we should have not done this in a point release - all good to know in hindsight. Anyway, it isn't clear that reverting after a year is going to help things so I'm thinking of just documenting for now with a Jira issue. Apologies if this has already been raised elsewhere.
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #282: Some bulk refactorings
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/282#discussion_r107540485
--- Diff: src/main/org/codehaus/groovy/tools/shell/IO.java ---
@@ -130,7 +130,7 @@ public boolean isDebug() {
/**
* Flush both output streams.
*/
- public void flush() throws IOException {
+ public void flush() {
--- End diff --
removal of checked exception on public method
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #282: Some bulk refactorings
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/282#discussion_r107539033
--- Diff: src/main/org/codehaus/groovy/reflection/GeneratedMetaMethod.java ---
@@ -162,7 +162,7 @@ public static void saveDgmInfo(List<DgmMethodRecord> records, String file) throw
out.close();
}
- public static List<DgmMethodRecord> loadDgmInfo() throws IOException, ClassNotFoundException {
+ public static List<DgmMethodRecord> loadDgmInfo() throws IOException {
--- End diff --
removal of checked exception from public api
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #282: Some bulk refactorings
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/282#discussion_r107536754
--- Diff: src/main/groovy/lang/ProxyMetaClass.java ---
@@ -44,7 +42,7 @@
/**
* convenience factory method for the most usual case.
*/
- public static ProxyMetaClass getInstance(Class theClass) throws IntrospectionException {
+ public static ProxyMetaClass getInstance(Class theClass) {
--- End diff --
removal of checked exception in public method
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #282: Some bulk refactorings
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/282#discussion_r107536859
--- Diff: src/main/groovy/lang/ProxyMetaClass.java ---
@@ -53,7 +51,7 @@ public static ProxyMetaClass getInstance(Class theClass) throws IntrospectionExc
/**
* @param adaptee the MetaClass to decorate with interceptability
*/
- public ProxyMetaClass(MetaClassRegistry registry, Class theClass, MetaClass adaptee) throws IntrospectionException {
--- End diff --
removal of checked exception in public method
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #282: Some bulk refactorings
Posted by oreissig <gi...@git.apache.org>.
Github user oreissig commented on a diff in the pull request:
https://github.com/apache/groovy/pull/282#discussion_r107503611
--- Diff: src/main/groovy/lang/GroovyCodeSource.java ---
@@ -164,7 +164,7 @@ public GroovyCodeSource(URI uri) throws IOException {
this(uri.toURL());
}
- public GroovyCodeSource(URL url) throws IOException {
--- End diff --
d'oh, I totally forgot about that :(
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #282: Some bulk refactorings
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/282#discussion_r107540260
--- Diff: src/main/org/codehaus/groovy/tools/DgmConverter.java ---
@@ -37,7 +37,7 @@
public class DgmConverter implements Opcodes {
- public static void main(String[] args) throws IOException, ClassNotFoundException {
+ public static void main(String[] args) throws IOException {
--- End diff --
even though this is not likely to cause a problem, still removal of checked exception on public method
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #282: Some bulk refactorings
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/282#discussion_r107536241
--- Diff: src/main/groovy/grape/GrapeIvy.groovy ---
@@ -76,11 +76,11 @@ class GrapeIvy implements GrapeEngine {
Set<String> resolvedDependencies
Set<String> downloadedArtifacts
// weak hash map so we don't leak loaders directly
- Map<ClassLoader, Set<IvyGrabRecord>> loadedDeps = new WeakHashMap<ClassLoader, Set<IvyGrabRecord>>()
+ final Map<ClassLoader, Set<IvyGrabRecord>> loadedDeps = new WeakHashMap<ClassLoader, Set<IvyGrabRecord>>()
// set that stores the IvyGrabRecord(s) for all the dependencies in each grab() call
- Set<IvyGrabRecord> grabRecordsForCurrDependencies = new LinkedHashSet<IvyGrabRecord>()
+ final Set<IvyGrabRecord> grabRecordsForCurrDependencies = new LinkedHashSet<IvyGrabRecord>()
// we keep the settings so that addResolver can add to the resolver chain
- IvySettings settings
+ final IvySettings settings
--- End diff --
not sure those finals are ok, it means there will be no getter for these anymore. So I would like to hear from for example Paul that these are not supposed to have getter
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request #282: Some bulk refactorings
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/282#discussion_r107536629
--- Diff: src/main/groovy/lang/ObjectRange.java ---
@@ -270,7 +270,8 @@ protected int compareTo(Comparable first, Comparable second) {
/**
* protection against calls from Groovy
*/
- private void setSize(int size) {
+ @Deprecated
+ void setSize(int size) {
throw new UnsupportedOperationException("size must not be changed");
--- End diff --
why is it deprecated again?
---
If your project is set up for it, you can reply to this email and have your
reply appear on GitHub as well. If your project does not have this feature
enabled and wishes so, or if the feature is enabled but not working, please
contact infrastructure at infrastructure@apache.org or file a JIRA ticket
with INFRA.
---