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.
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Added regex filter field for TF display #1032
Added regex filter field for TF display #1032
Changes from 2 commits
a87a102
fe9e855
246e0d7
29fbdf8
76b33d0
bb6e97f
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not opposed to this code as-is, but I'm just wondering; can this exception actually happen given the
RegexValidator
we have installed?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It feels to me like these loops are going to be doing an unnecessary amount of work, in both the case where filtering is used and where it is not used. I'm particularly worried about large TF trees.
For the first loop, if someone is not using the filtering, we are still going to be iterating over every single frame to just ignore it. It seems like if both the whitelist and the blacklist are empty, we should skip the loop completely.
If the user is using the filtering, then we are iterating over the list of frames 3 times; once to apply the regex, once to sort, and once to build up the current frames. It seems to me that we should be able to get away with only iterating once. That is, we have a loop, and in the loop body we compare it to the whitelist and blacklist. If it is not to be shown because it is explicitly in the blacklist, or it is not in the whitelist (or it is empty), we do nothing more. If we are going to show it, then we create/update the frame for it, and insert it into
current_frames
. Sincecurrent_frames
is a set anyway (which is sorted: https://en.cppreference.com/w/cpp/container/set), there is no need for a separate sort step. We can do a similar thing for the case where the whitelist and blacklist are empty, and skip the additional sort step.At least, that's how it seems to me. Does that make sense to you? Am I possibly missing something with the separate
sort
and "insert into sorted set" step?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what do you think @clalancette ? 246e0d7