[GitHub] [commons-collections] garydgregory commented on pull request #258: Simplify bloom filters
garydgregory commented on PR #258: URL: https://github.com/apache/commons-collections/pull/258#issuecomment-1156740141 Merged! TY @Claudenw -- 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. To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-collections] garydgregory commented on pull request #258: Simplify bloom filters
garydgregory commented on PR #258: URL: https://github.com/apache/commons-collections/pull/258#issuecomment-1154262000 > The drawback of merging now is losing all the comments that are as yet unresolved on this PR. Hi @aherbert I see that JIRA issues are now open to track tasks: https://issues.apache.org/jira/browse/COLLECTIONS-816 through https://issues.apache.org/jira/browse/COLLECTIONS-819 Is that good enough that we can merge? -- 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. To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-collections] garydgregory commented on pull request #258: Simplify bloom filters
garydgregory commented on PR #258: URL: https://github.com/apache/commons-collections/pull/258#issuecomment-1151144559 > Yes I would like to merge this request and pick up the any issues as new issues to work through as smaller changes. That sounds good to me. -- 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. To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-collections] garydgregory commented on pull request #258: Simplify bloom filters
garydgregory commented on PR #258: URL: https://github.com/apache/commons-collections/pull/258#issuecomment-1151129192 I am OK to merge this PR and continue with a new PR. @Claudenw "I would like to see the change sets reduced as we resolve the remaining issues." Does that mean you want to update this PR or continue with a new one? @aherbert WDYT? -- 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. To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-collections] garydgregory commented on pull request #258: Simplify bloom filters
garydgregory commented on PR #258: URL: https://github.com/apache/commons-collections/pull/258#issuecomment-1149234665 Note: The build is failing. -- 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. To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-collections] garydgregory commented on pull request #258: Simplify bloom filters
garydgregory commented on PR #258: URL: https://github.com/apache/commons-collections/pull/258#issuecomment-1147491649 Hi All. Ping. Where are we on this PR? -- 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. To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-collections] garydgregory commented on pull request #258: Simplify bloom filters
garydgregory commented on pull request #258: URL: https://github.com/apache/commons-collections/pull/258#issuecomment-1017712873 > @aherbert , @dota17 , @garydgregory , @kinow > > Can I get a review and possible merge for this pull? It has the changes requested and is much cleaner than what is there now. If there are more things to clean up I would like to do it as smaller pull requests. > > Thank you Fine w me, @aherbert ? -- 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. To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-collections] garydgregory commented on pull request #258: Simplify bloom filters
garydgregory commented on pull request #258: URL: https://github.com/apache/commons-collections/pull/258#issuecomment-1011254728 I'm OK with bringing this in pending reviews from previous commenters @aherbert @kinow. Note: This is new code so we do not need to worry about compatibility. -- 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. To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-collections] garydgregory commented on pull request #258: Simplify bloom filters
garydgregory commented on pull request #258: URL: https://github.com/apache/commons-collections/pull/258#issuecomment-996153162 My suggestion boils down to one of these classes being a kind of stream such that it does not need to implement forEachIndex. I am dealing with Log4j ATM, so I can look at this later, when the code is in git master or wherever it ends up being (later). -- 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. To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
[GitHub] [commons-collections] garydgregory commented on pull request #258: Simplify bloom filters
garydgregory commented on pull request #258:
URL:
https://github.com/apache/commons-collections/pull/258#issuecomment-994870141
Hi All,
I agree on initially minimizing exposure of internal utilities WRT BitMap.
As @aherbert mentioned, this is an anti-pattern for sure:
```
@Override
public boolean contains(IndexProducer indexProducer) {
try {
indexProducer.forEachIndex(idx -> {
if (!indices.contains(idx)) {
throw new NoMatchException();
}
});
return true;
} catch (NoMatchException e) {
return false;
}
}
```
It sounds like something where a Stream API can be used. But then why not
delegate the implementation of contains to IndexProducer? But... then
IndexProducer looks like:
```
...
public interface IndexProducer {
/**
* Each index is passed to the consumer.
* Any exceptions thrown by the action are relayed to the caller.
*
* Indices ordering is not guaranteed
*
* @param consumer the action to be performed for each non-zero bit
index.
* @throws NullPointerException if the specified action is null
*/
void forEachIndex(IntConsumer consumer);
...
```
And that does not seem best at first glance because Stream#forEach() can
already take an IntConsumer as a parameter but this is not a Stream.
Anyway, I am OK bringing this in with or without the int to long change
because it will be easier to further experiment and maybe improve the design
regarding Streams and implementations of methods. I'll leave it to @aherbert to
approve the PR or ask for more changes.
--
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.
To unsubscribe, e-mail: issues-unsubscr...@commons.apache.org
For queries about this service, please contact Infrastructure at:
us...@infra.apache.org
