Skip to content
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

Scheduler: allow negation of regular expressions in plan class XML specs (standard) #4529

Open
wants to merge 1 commit into
base: dpa_regex
Choose a base branch
from

Conversation

bema-aei
Copy link
Contributor

This MR is an extension of #4390 with the changes discussed but not (yet) implemented there, i.e. using the standard (?!regex) as proposed by @Rytiss instead of the simple but non-standard !regex proposed by me and implemented by @davidpanderson . The branch is based on David's dpa_regex with an extra commit.

@Rytiss
Copy link
Contributor

Rytiss commented Oct 5, 2021

I think this should work, but I'm not a C++ guy so I won't merge this.

Thanks for taking your time to mod the original PR!

@bema-aei
Copy link
Contributor Author

bema-aei commented Oct 5, 2021

@Rytiss I think this implementation / modification comes closest to our original intentions of negation of regex. I wanted to have this ready before your original proposal becomes implemented. Thanks @davidpanderson for taking the time for the previous implementation, factoring out these regex as a separate datatype was valuable.

However I didn't test this yet very much, due to lack of time and opportunity, and I didn't look much around in that code, in particular where the C-Strings that are manipulated there originate from. Certainly there is potential for an accidental error (probably off-by-one) in my modification. And we recently came across trouble with storing (char*) pointers to C strings converted from C++ strings with c_str(). In modern clib versions the lifetime of these converted strings seems different than in previous versions, a pointer to such a string conversion buffer that is stored permanently may give unexpected results after some time, i.e. after other such conversions had happened. Maybe we should use strdup() to preserve the original regex string.

@AenBleidd AenBleidd self-requested a review October 5, 2021 10:49
@AenBleidd
Copy link
Member

@bema-aei, I'll check c++ code (without logic behind it)

@AenBleidd AenBleidd self-assigned this Oct 7, 2021
@@ -33,13 +33,17 @@ int REGEX_CLAUSE::init(const char* p) {
present = true;
negate = false;
expr = p;
Copy link
Member

Choose a reason for hiding this comment

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

Here copy constructor will be called

p++;
if (!strncmp(p,"(?!",3) && p[strlen(p)-1] == ')') {
p += 3;
p[strlen(p)-1] = '\0'; // don't parse trailing ')'
Copy link
Member

Choose a reason for hiding this comment

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

At this point data at expr will not be changed, because you're modifying the original data piece that has nothing common with expr at all.
If this is done by intention - then it's fine.

negate = true;
}
if (regcomp(&regex, p, REG_EXTENDED|REG_NOSUB) ) {
return ERR_XML_PARSE;
}
if (negate) {
p[strlen(p)] = ')'; // restore trailing ')'
Copy link
Member

Choose a reason for hiding this comment

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

Here the same as above: data stored in expr will not be modified.

@AenBleidd
Copy link
Member

Reference: https://godbolt.org/z/r1szzY9rr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Backlog
Development

Successfully merging this pull request may close these issues.

4 participants