Partial flush and index locations#326
Open
myrrhlin wants to merge 3 commits intoPerl-Critic:masterfrom
Open
Conversation
Author
|
these tests document desired behavior with 'partial' _flush_locations (for when the developer knows the place where the cached locations are bad), and index_locations (when -some- locations, presumably good, already exist). these behaviors are not documented in POD, only in source code. but they don't currently work!
When it exists, the value of $token->{_location} is an arrayref,
so line 804 elicits a fatal error:
Not a SCALAR reference
This is currently dead code -- no existing execution path in PPI actually
triggers this block. But we can make it work as intended.
This resolves part of issue Perl-Critic#325
As described in issue Perl-Critic#325, this method leaves alone existing locations, up to the first element that is missing one, then it recalculates locations for the rest of the document. This is a performance optimization. However, the _add_location method needs to be given the previous (last good) location (passed as the first argument, $start) to calculate the next one. In the partial reindexing case, where a document has its first element with a location, then _add_location is passed undef for $start, which elicits the warnings: Use of uninitialized value in numeric eq (==) at ../PPI/Document.pm line 749. Use of uninitialized value in addition (+) at ../PPI/Document.pm line 794. Use of uninitialized value in addition (+) at ../PPI/Document.pm line 723. ..and the calculated locations are all wrong. This commit fixes the partial indexing. A side effect of this solution is that all the recalculated element locations retain the original filename, even if they didn't originate in that file.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Resolves issue #325
PPI:Document->index_locationscontains code to perform partial reindexing -- that is, it only recalculates locations for the first element seen to be missing one, and all subsequent elements. Existing locations at the beginning of the document are considered still good. This is a performance optimization. This would be really handy, for example, if one were to use e.g. areplacemethod to change part of an existing document with new source text, which can make the cached locations for all subsequent elements invalid. This behavior is undocumented in the POD.PPI:Document->flush_locationscan be used to flush all locations from the document; it leans on the private method PPI:Element->_flush_locations to do the job. However, the latter contains code to do "partial flushing" of locations -- that is, flush the cached location from the invoking instance and all subsequent elements, but leaving prior element locations intact. This would be really handy to ensure that a laterDocument->index_locationscall only does a partial reindex. This behavior is also undocumented in the POD.Despite the existing code, neither of these intended behaviors work, because each has a minor bug. There are (obviously) no tests of these desired behaviors.