You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@groovy.apache.org by kirill-vlasov <gi...@git.apache.org> on 2016/01/04 12:23:16 UTC
[GitHub] groovy pull request: Two branches in the same conditional structur...
GitHub user kirill-vlasov opened a pull request:
https://github.com/apache/groovy/pull/232
Two branches in the same conditional structure should not have exactly the same implementation
This pull request is focused on resolving occurrences of Sonar rule squid:S1871 - Two branches in the same conditional structure should not have exactly the same implementation
You can find more information about the issue here:
https://dev.eclipse.org/sonar/coding_rules#q=squid:S1871
Please let me know if you have any questions.
Kirill Vlasov
You can merge this pull request into a Git repository by running:
$ git pull https://github.com/DevFactory/groovy staging/Two-branches-in-the-same-conditional-structure-should-not-have-exactly-the-same-implementation-fix-3
Alternatively you can review and apply these changes as the patch at:
https://github.com/apache/groovy/pull/232.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 #232
----
commit dd04e9147e98d5a5568f1fd41b784577b28c2c4c
Author: Kirill Vlasov <ki...@devfactory.com>
Date: 2015-12-30T05:58:47Z
Fixing squid:S1871 - Two branches in the same conditional structure should not have exactly the same implementation
----
---
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: Two branches in the same conditional structur...
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/232#discussion_r48809410
--- Diff: src/main/org/codehaus/groovy/classgen/asm/OperandStack.java ---
@@ -336,14 +336,12 @@ private void doConvertAndCast(ClassNode targetType, boolean coerce) {
return;
}
box();
- } else if (primTop) {
- // top is primitive, target is not
- // so box and do groovy cast
- controller.getInvocationWriter().castToNonPrimitiveIfNecessary(top, targetType);
} else if (primTarget) {
// top is not primitive so unbox
// leave that BH#doCast later
} else {
+ // top is primitive, target is not
+ // so box and do groovy cast
--- End diff --
I am thinking this comment should be improved. It covers both the case the comment mentions and the default case. Let's see if Jochen or anyone else has thoughts on what that default branch is supposed to do with respect to how best to rephrase if needed.
---
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: Two branches in the same conditional structur...
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/232#discussion_r48842357
--- Diff: src/main/org/codehaus/groovy/classgen/asm/OperandStack.java ---
@@ -336,14 +336,12 @@ private void doConvertAndCast(ClassNode targetType, boolean coerce) {
return;
}
box();
- } else if (primTop) {
- // top is primitive, target is not
- // so box and do groovy cast
- controller.getInvocationWriter().castToNonPrimitiveIfNecessary(top, targetType);
} else if (primTarget) {
// top is not primitive so unbox
// leave that BH#doCast later
} else {
+ // top is primitive, target is not
+ // so box and do groovy cast
--- End diff --
the comment needs a change... "// top might be primitive, target is not" and "// so let invocation writer box if needed and do groovy cast otherwise". I think this case appeared to due to moving some functionality into the invocation writer, I think before there has been really two different actions. Well.. Still can make this change with the changed comment then of course ;)
---
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: Two branches in the same conditional structur...
Posted by blackdrag <gi...@git.apache.org>.
Github user blackdrag commented on a diff in the pull request:
https://github.com/apache/groovy/pull/232#discussion_r48842494
--- Diff: src/main/org/codehaus/groovy/classgen/asm/sc/StaticInvocationWriter.java ---
@@ -602,7 +602,7 @@ private static void pushZero(final MethodVisitor mv, final ClassNode type) {
boolean isInt = ClassHelper.int_TYPE.equals(type);
boolean isShort = ClassHelper.short_TYPE.equals(type);
boolean isByte = ClassHelper.byte_TYPE.equals(type);
- if (isInt || isShort || isByte) {
+ if (isInt || isShort || isByte || ClassHelper.boolean_TYPE.equals(type)) {
--- End diff --
actuallly, the "|| ClassHelper.boolean_TYPE.equals(type)" should be removed. isByte contains this information already.
---
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: Two branches in the same conditional structur...
Posted by asfgit <gi...@git.apache.org>.
Github user asfgit closed the pull request at:
https://github.com/apache/groovy/pull/232
---
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.
---