You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@geode.apache.org by GitBox <gi...@apache.org> on 2020/07/24 23:12:38 UTC
[GitHub] [geode] DonalEvans opened a new pull request #5400: GEODE-7864: Fix several LGTM warnings
DonalEvans opened a new pull request #5400:
URL: https://github.com/apache/geode/pull/5400
- This commit addresses language-features, logic, useless-code and
readability warnings
- The change to CompactRangeIndex may be fixing a bug/incorrectness in
the code
Authored-by: Donal Evans <do...@vmware.com>
Thank you for submitting a contribution to Apache Geode.
In order to streamline the review of the contribution we ask you
to ensure the following steps have been taken:
### For all changes:
- [ ] Is there a JIRA ticket associated with this PR? Is it referenced in the commit message?
- [ ] Has your PR been rebased against the latest commit within the target branch (typically `develop`)?
- [ ] Is your initial contribution a single, squashed commit?
- [ ] Does `gradlew build` run cleanly?
- [ ] Have you written or updated unit tests to verify your changes?
- [ ] If adding new dependencies to the code, are these dependencies licensed in a way that is compatible for inclusion under [ASF 2.0](http://www.apache.org/legal/resolved.html#category-a)?
### Note:
Please ensure that once the PR is submitted, check Concourse for build issues and
submit an update to your PR as soon as possible. If you need help, please send an
email to dev@geode.apache.org.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5400: GEODE-7864: Fix several LGTM warnings
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5400:
URL: https://github.com/apache/geode/pull/5400#issuecomment-663777444
This pull request **fixes 7 alerts** when merging 0418008cc33c2e351371088ad78b447d0e84da75 into 067194e8f82414b6103c4335beb6567947edcfbc - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-18be54c5a4d57e413d1a6c87eaab39d3f7fd7cdd)
**fixed alerts:**
* 3 for Non\-synchronized override of synchronized method
* 1 for Useless comparison test
* 1 for Superfluous trailing arguments
* 1 for Missing space in string concatenation
* 1 for Useless null check
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans merged pull request #5400: GEODE-7864: Fix several LGTM warnings
Posted by GitBox <gi...@apache.org>.
DonalEvans merged pull request #5400:
URL: https://github.com/apache/geode/pull/5400
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] nabarunnag commented on a change in pull request #5400: GEODE-7864: Fix several LGTM warnings
Posted by GitBox <gi...@apache.org>.
nabarunnag commented on a change in pull request #5400:
URL: https://github.com/apache/geode/pull/5400#discussion_r461895424
##########
File path: geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java
##########
@@ -939,7 +939,7 @@ protected boolean evaluateEntry(IndexInfo indexInfo, ExecutionContext context, O
// either of them is null and operator is other than == or !=
if (result == QueryService.UNDEFINED) {
// Undefined is added to results for != conditions only
- if (operator != OQLLexerTokenTypes.TOK_NE || operator != OQLLexerTokenTypes.TOK_NE_ALT) {
Review comment:
judging by this comment.
`// Undefined is added to results for != conditions only`
##########
File path: geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java
##########
@@ -939,7 +939,7 @@ protected boolean evaluateEntry(IndexInfo indexInfo, ExecutionContext context, O
// either of them is null and operator is other than == or !=
if (result == QueryService.UNDEFINED) {
// Undefined is added to results for != conditions only
- if (operator != OQLLexerTokenTypes.TOK_NE || operator != OQLLexerTokenTypes.TOK_NE_ALT) {
Review comment:
This seems like a behavior change, sorry if i am missing something.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] DonalEvans commented on a change in pull request #5400: GEODE-7864: Fix several LGTM warnings
Posted by GitBox <gi...@apache.org>.
DonalEvans commented on a change in pull request #5400:
URL: https://github.com/apache/geode/pull/5400#discussion_r461909472
##########
File path: geode-core/src/main/java/org/apache/geode/cache/query/internal/index/CompactRangeIndex.java
##########
@@ -939,7 +939,7 @@ protected boolean evaluateEntry(IndexInfo indexInfo, ExecutionContext context, O
// either of them is null and operator is other than == or !=
if (result == QueryService.UNDEFINED) {
// Undefined is added to results for != conditions only
- if (operator != OQLLexerTokenTypes.TOK_NE || operator != OQLLexerTokenTypes.TOK_NE_ALT) {
Review comment:
It is a change in behaviour, but I think it's a change that introduces the originally intended behaviour. The previous logic always evaluated to true, which made me think that there might have been a mistake in it. The comment about "for != conditions only" I think is referring to the `TOK_NE` and `TOK_NE_ALT` types (not equal and not equal alt), not that the operator should be not equal to them.
Previously, any time the result was `UNDEFINED` it would be added to the results, but with the change, it's only added to the results if the operator is `TOK_NE` or `TOK_NE_ALT`, which sounds from the comment to be what was originally intended. All of the tests are passing here with this change, and I ran some other tests privately that also hit no issues, so I'm fairly confident that this isn't breaking anything.
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5400: GEODE-7864: Fix several LGTM warnings
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5400:
URL: https://github.com/apache/geode/pull/5400#issuecomment-663912138
This pull request **fixes 18 alerts** when merging c32020eb8f49ad26aebbf88706793f70b49dbacc into 067194e8f82414b6103c4335beb6567947edcfbc - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-9d96c76acbf0f5124a5aed7cafda33d043853745)
**fixed alerts:**
* 9 for DOM text reinterpreted as HTML
* 3 for Non\-synchronized override of synchronized method
* 1 for Useless comparison test
* 1 for Superfluous trailing arguments
* 1 for Missing space in string concatenation
* 1 for Useless null check
* 1 for Double escaping or unescaping
* 1 for Incomplete string escaping or encoding
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
[GitHub] [geode] lgtm-com[bot] commented on pull request #5400: GEODE-7864: Fix several LGTM warnings
Posted by GitBox <gi...@apache.org>.
lgtm-com[bot] commented on pull request #5400:
URL: https://github.com/apache/geode/pull/5400#issuecomment-663893164
This pull request **introduces 1 alert** and **fixes 17** when merging 82c87573e139d5860f97cd1c85dbccc7ddb90e30 into 067194e8f82414b6103c4335beb6567947edcfbc - [view on LGTM.com](https://lgtm.com/projects/g/apache/geode/rev/pr-872796047847ee7a318d33587ab99c428b78c44c)
**new alerts:**
* 1 for Double escaping or unescaping
**fixed alerts:**
* 9 for DOM text reinterpreted as HTML
* 3 for Non\-synchronized override of synchronized method
* 1 for Useless comparison test
* 1 for Superfluous trailing arguments
* 1 for Missing space in string concatenation
* 1 for Useless null check
* 1 for Double escaping or unescaping
----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
For queries about this service, please contact Infrastructure at:
users@infra.apache.org