[GitHub] ant-ivy pull request #53: suppressed warnings

2017-08-31 Thread asfgit
Github user asfgit closed the pull request at:

https://github.com/apache/ant-ivy/pull/53


---
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.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy pull request #53: Suppressed warnings

2017-07-26 Thread jaikiran
Github user jaikiran commented on a diff in the pull request:

https://github.com/apache/ant-ivy/pull/53#discussion_r129757849
  
--- Diff: 
src/java/org/apache/ivy/plugins/conflict/LatestCompatibleConflictManager.java 
---
@@ -85,14 +85,12 @@ public LatestCompatibleConflictManager(String name, 
LatestStrategy strategy) {
 if (versionMatcher.isDynamic(mrid)) {
 while (iter.hasNext()) {
 IvyNode other = iter.next();
-if (versionMatcher.isDynamic(other.getResolvedId())) {
+if (versionMatcher.isDynamic(other.getResolvedId())
+|| !versionMatcher.accept(mrid, 
other.getResolvedId())
+&& !handleIncompatibleConflict(parent, conflicts, 
node, other)) {
--- End diff --

You are right. Although I was aware of short circuiting, I had a wrong 
semantic in mind about it. I was under the impression, the short circuiting 
happens "smartly" depending on what other operators are present in that 
expression. I read up the JLS (section 15.24)[1] now and as you note, the right 
side of the `||` will be completely ignored (irrespective of what additional 
operators exist in that expression) if the left side returns true. So this 
change of the `if` block is fine.

[1] http://docs.oracle.com/javase/specs/jls/se7/html/jls-15.html#jls-15.24


---
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.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy pull request #53: Suppressed warnings

2017-07-26 Thread twogee
Github user twogee commented on a diff in the pull request:

https://github.com/apache/ant-ivy/pull/53#discussion_r129668076
  
--- Diff: src/java/org/apache/ivy/plugins/report/XmlReportParser.java ---
@@ -71,132 +71,139 @@
 
--- End diff --

Here's a "diff -wc"-style diff

[XmlReporteParser.zip](https://github.com/apache/ant-ivy/files/1177761/XmlReporteParser.zip)



---
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.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy pull request #53: Suppressed warnings

2017-07-26 Thread twogee
Github user twogee commented on a diff in the pull request:

https://github.com/apache/ant-ivy/pull/53#discussion_r129663139
  
--- Diff: 
src/java/org/apache/ivy/plugins/conflict/LatestCompatibleConflictManager.java 
---
@@ -85,14 +85,12 @@ public LatestCompatibleConflictManager(String name, 
LatestStrategy strategy) {
 if (versionMatcher.isDynamic(mrid)) {
 while (iter.hasNext()) {
 IvyNode other = iter.next();
-if (versionMatcher.isDynamic(other.getResolvedId())) {
+if (versionMatcher.isDynamic(other.getResolvedId())
+|| !versionMatcher.accept(mrid, 
other.getResolvedId())
+&& !handleIncompatibleConflict(parent, conflicts, 
node, other)) {
--- End diff --

[It still does](http://introcs.cs.princeton.edu/java/11precedence/). 
There's a difference between `&&` and `||` vs `&` and `|`, see "short 
circuiting".


---
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.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy pull request #53: Suppressed warnings

2017-07-26 Thread twogee
Github user twogee commented on a diff in the pull request:

https://github.com/apache/ant-ivy/pull/53#discussion_r129660986
  
--- Diff: src/java/org/apache/ivy/plugins/report/XmlReportParser.java ---
@@ -71,132 +71,139 @@
 
--- End diff --

It's a change from if ... else if ... else if ... else if ... to a switch, 
which changes indentation and messes up git diff. I will rebase and rearrange 
the remaining commits.


---
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.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy pull request #53: Suppressed warnings

2017-07-26 Thread jaikiran
Github user jaikiran commented on a diff in the pull request:

https://github.com/apache/ant-ivy/pull/53#discussion_r129587968
  
--- Diff: 
src/java/org/apache/ivy/plugins/conflict/LatestCompatibleConflictManager.java 
---
@@ -85,14 +85,12 @@ public LatestCompatibleConflictManager(String name, 
LatestStrategy strategy) {
 if (versionMatcher.isDynamic(mrid)) {
 while (iter.hasNext()) {
 IvyNode other = iter.next();
-if (versionMatcher.isDynamic(other.getResolvedId())) {
+if (versionMatcher.isDynamic(other.getResolvedId())
+|| !versionMatcher.accept(mrid, 
other.getResolvedId())
+&& !handleIncompatibleConflict(parent, conflicts, 
node, other)) {
--- End diff --

This change doesn't look right. The previous logic would have returned 
`null` if the `versionMatcher.isDynamic` returned true. With this change, it 
will require an additional `&&` to be satisfied for the 
`!handlerIncompatibleConflict...`


---
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.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy pull request #53: Suppressed warnings

2017-07-26 Thread jaikiran
Github user jaikiran commented on a diff in the pull request:

https://github.com/apache/ant-ivy/pull/53#discussion_r129593699
  
--- Diff: src/java/org/apache/ivy/plugins/report/XmlReportParser.java ---
@@ -71,132 +71,139 @@
 
--- End diff --

This file has a large change which appears to be reordering/restructuring 
the code logic. Given the nature of this diff, it's difficult to review it and 
see if this change introduces any changes in the logic or introduces any 
regressions. Plus this class is one of the very core ones. Also, I'm not too 
sure if the existing test cases cover for this level of change.

Given all this, I'm not confident that these changes in this specific class 
are worth it, at least at the moment. Unless of course, some one else can 
review this specific class and approve it, would you be willing to undo the 
changes to this one class in this PR?



---
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.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org



[GitHub] ant-ivy pull request #53: Suppressed warnings

2017-07-18 Thread twogee
GitHub user twogee opened a pull request:

https://github.com/apache/ant-ivy/pull/53

Suppressed warnings

As described in previous PR, all compiler warnings are annotated.
This PR removes a few of them, along with some refactoring.
Changes are grouped in separate commits which are hopefully self-describing.

You can merge this pull request into a Git repository by running:

$ git pull https://github.com/twogee/ant-ivy suppressed-warnings

Alternatively you can review and apply these changes as the patch at:

https://github.com/apache/ant-ivy/pull/53.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 #53


commit 3781faf7238a3a9dc185a863f8b0582a7c28d1b4
Author: twogee 
Date:   2017-07-18T12:08:13Z

cosmetics

commit ba221a1b4323aa59137886b48c70d2ba3480325f
Author: twogee 
Date:   2017-07-18T12:09:12Z

get rid of suppressed warnings

commit bd0f075d2cecf7d6d86c9ce92ed6727438f06f05
Author: twogee 
Date:   2017-07-18T12:09:52Z

refactor

commit 2f0cd05b6ab4bb84eff3b21ebbc966de23e740f0
Author: twogee 
Date:   2017-07-18T12:10:08Z

fix exceptions




---
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.
---

-
To unsubscribe, e-mail: dev-unsubscr...@ant.apache.org
For additional commands, e-mail: dev-h...@ant.apache.org