Add UnwrappingReuseStrategy for AnalyzerWrapper #14154
Conversation
Add a new reuse strategy WrappingReuseStrategy that consults the wrapped analyzer's strategy to decide if components can be reused or need to be updated. Add an optional constructor to CompletionAnalayzer that uses this strategy.
| public CompletionAnalyzer( | ||
| Analyzer analyzer, | ||
| boolean preserveSep, | ||
| boolean preservePositionIncrements, | ||
| ReuseStrategy fallbackStrategy) { | ||
| super(new WrappingReuseStrategy(fallbackStrategy)); | ||
| // häckidy-hick-hack, because we cannot call super() with a reference to "this": | ||
| ((WrappingReuseStrategy) getReuseStrategy()).setUp(this, analyzer, wrappedComponents); | ||
| this.analyzer = analyzer; | ||
| this.preserveSep = preserveSep; | ||
| this.preservePositionIncrements = preservePositionIncrements; | ||
| this.maxGraphExpansions = ConcatenateGraphFilter.DEFAULT_MAX_GRAPH_EXPANSIONS; | ||
| } |
There was a problem hiding this comment.
/**
* JAVA DOCS about when you should use this and what it does
*/
public static CompletionAnalyzer withWrappingReuseStrategy(
Analyzer analyzer,
boolean preserveSep,
boolean preservePositionIncrements,
ReuseStrategy fallbackStrategy) {
CompletionAnalyzer completionAnalyzer = new CompletionAnalyzer(analyzer, preserveSep, preservePositionIncrements, fallbackStrategy);
((WrappingReuseStrategy)completionAnalyzer.getReuseStrategy()).setUp(completionAnalyzer, analyzer, completionAnalyzer.wrappedComponents);
return completionAnalyzer;
}
What if we made the ctor private, removed the setup hack and did it instead within a public static method?
| public void setUp( | ||
| AnalyzerWrapper wrapper, | ||
| Analyzer wrappedAnalyzer, | ||
| CloseableThreadLocal<TokenStreamComponents> wrappedComponents) { | ||
| this.wrapper = wrapper; | ||
| this.wrappedAnalyzer = wrappedAnalyzer; | ||
| this.wrappedComponents = wrappedComponents; | ||
| } |
There was a problem hiding this comment.
This bugs me. We allow a reuse strategy to be mutated after construction. It seems ready to cause a bug.
|
|
||
| @Override | ||
| public TokenStreamComponents getReusableComponents(Analyzer analyzer, String fieldName) { | ||
| if (analyzer == wrapper) { |
There was a problem hiding this comment.
Do we really need to know the instance, or can we do Analyzer analyzer instanceof WrapperAnalyzer wrapperAnalyzer then get the strategy from wrapperAnalyzer.getWrappedAnalyzer(fieldName).getReuseStategy().getReusableComponents...?
| public void setReusableComponents( | ||
| Analyzer analyzer, String fieldName, TokenStreamComponents components) { | ||
| if (analyzer == wrapper) { | ||
| setStoredValue(analyzer, components); | ||
| wrappedAnalyzer | ||
| .getReuseStrategy() | ||
| .setReusableComponents(wrappedAnalyzer, fieldName, wrappedComponents.get()); | ||
| } else { | ||
| fallbackStrategy.setReusableComponents(analyzer, fieldName, components); | ||
| } |
There was a problem hiding this comment.
Do we really need to know the instance, or can we do Analyzer analyzer instanceof WrapperAnalyzer wrapperAnalyzer then get the wrapped analyzer via getWrappedAnalyzer(fieldName)?
| * be re-created. During components creation, this analyzer must store the wrapped analyzer's | ||
| * components in {@code wrappedComponents} local thread variable. | ||
| */ | ||
| public static final class WrappingReuseStrategy extends ReuseStrategy { |
There was a problem hiding this comment.
It seems to me that the better name is UnWrapping as if we find that the analyzer is a wrapped analyzer, we unwrap it and use the underlying analyzer?
| public void setUp( | ||
| AnalyzerWrapper wrapper, | ||
| Analyzer wrappedAnalyzer, | ||
| CloseableThreadLocal<TokenStreamComponents> wrappedComponents) { | ||
| this.wrapper = wrapper; | ||
| this.wrappedAnalyzer = wrappedAnalyzer; | ||
| this.wrappedComponents = wrappedComponents; | ||
| } |
There was a problem hiding this comment.
Ah, I see that the "wrappedComponents" is key here. its tricky, as the analyzer provides access directly to storedValue and there is no way to override :(.
That seems like a bad API already, but we don't want to change all these long lived interfaces unnecessarily.
|
I don't like that |
|
@benwtrent Thanks for the review, I am not happy with the design either, will see how I can incorporate your feedback.
@jpountz Thanks for looking. The idea of having an extra thread-local is for AnalyzerWrapper to keep track of |
|
@jpountz @benwtrent I've addressed your comments in the last commit, please continue to review |
Add a new reuse strategy WrappingReuseStrategy that consults
he wrapped analyzer's strategy to decide if components can be
reused or need to be updated.
Add an optional constructor to CompletionAnalayzer that uses this strategy.