-
Notifications
You must be signed in to change notification settings - Fork 381
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
libvcc: Fix STRING/STRANDS comparison without STRING folding #4148
Conversation
bugwash @walid-git was asked to review, thank you in advance. |
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.
Even though the changes generate syntactically correct C code and that the STRING/STRANDS comparison error is gone, there is still something wrong here:
With these changes, the following VCL code:
if ("string" == debug.return_strands("string")) {
...
}
Will generate this C code:
if (
(0 == VRT_CompareStrands(TOSTRAND("string"), vrt_null_strands))
) {
...
The STRANDS part of debug.return_strands("string")
is being translated to vrt_null_strands
which is wrong. I need to look closer to find which part of the PR is causing this, but that might be related to the first commit. We should also improve test coverage to not only test the syntactic part but also the semantic.
3a368c5
to
4dd1eee
Compare
@walid-git thank you very much for catching this significant regression. I have force-pushed an update, the delta is diff --git a/lib/libvcc/vcc_expr.c b/lib/libvcc/vcc_expr.c
index a348e9b83..cd4b03c2f 100644
--- a/lib/libvcc/vcc_expr.c
+++ b/lib/libvcc/vcc_expr.c
@@ -159,7 +159,11 @@ static void
vcc_strands_edit(const struct expr *e1, const struct expr *e2)
{
- if (e2->nstr == 0)
+ assert(e2->fmt == STRANDS || e2->fmt == STRINGS);
+
+ if (e2->fmt == STRANDS)
+ VSB_cat(e1->vsb, VSB_data(e2->vsb));
+ else if (e2->nstr == 0)
VSB_printf(e1->vsb, "vrt_null_strands");
else if (e2->nstr == 1)
VSB_printf(e1->vsb, "TOSTRAND(%s)", VSB_data(e2->vsb)); edit: the first force-push applied the fix to the wrong commit |
Fixes varnishcache#4146 Reverts 42a7ecd Reverts the code change from b77b709
4dd1eee
to
270c873
Compare
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.
Thanks @nigoroll. This looks good to me now. I left a comment for test coverage improvement.
by moving test VCL from vcl_recv {} to vcl_init {} As suggested by walid here: varnishcache#4148 (comment)
this almost halves runtime of the test case
fa02fce
to
808a8dd
Compare
I do not consider myself enough of an expert on VCC and would appreciate reviews by those who do.
Side note: STRING vs STRINGS vs. STRANDS is confusing
Fixes #4146