[GitHub] groovy pull request: Two branches in the same conditional structur...
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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
[GitHub] groovy pull request: Two branches in the same conditional structur...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request: Two branches in the same conditional structur...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[GitHub] groovy pull request: Two branches in the same conditional structur...
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 infrastruct...@apache.org or file a JIRA ticket
with INFRA.
---
[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 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 infrastruct...@apache.org or file a JIRA ticket with INFRA. ---
