Skip to content
This repository has been archived by the owner on May 12, 2020. It is now read-only.

Add support for conditional matching filters based on tags #192

Merged
merged 2 commits into from
Mar 13, 2019

Conversation

bbondy
Copy link
Member

@bbondy bbondy commented Mar 3, 2019

See here for a description of the task:
Fix #191

Copy link
Contributor

@emerick emerick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, just had one question about the buffer management (which could be totally innocuous, just wasn't sure).

@@ -1472,6 +1495,15 @@ int serializeFilters(char * buffer, size_t bufferSizeAvail,
}
bufferSize++;

if (f->tagLen > 0) {
if (buffer) {
buffer[bufferSize] = '#';
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it safe to assume the buffer has the space available for this additional data? Wasn't quite sure how that side of things works.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yep how it works is it calls the function 2 times. The first with nullptr buffer to get the size. The second it specifies a buffer of that size.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, got it!

Copy link
Contributor

@SergeyZhukovsky SergeyZhukovsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

there are risky changes with memory management. I hope unit tests covered it. Looks good otherwise.

@bbondy bbondy force-pushed the condition-tagging branch 3 times, most recently from d8ec43b to e8b9af3 Compare March 10, 2019 02:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add support for tagging ad-block rules
3 participants