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

vdef: Add TFOREACH() macro for txt #4194

Merged
merged 2 commits into from
Sep 24, 2024
Merged

Conversation

dridi
Copy link
Member

@dridi dridi commented Sep 23, 2024

Random idea of the day.

include/vdef.h Outdated
@@ -266,6 +266,8 @@ typedef struct {
#define Tstr(s) (/*lint -e(446)*/ (txt){(s), (s) + strlen(s)})
#define Tstrcmp(t, s) (strncmp((t).b, (s), Tlen(t)))

#define TFOREACH(c, t) for ((c) = (t).b; (c) < (t).e; (c)++)
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice find.
Could we instead go for #define Tforeach(c, t) ? or #define TXT_FOREACH(c, t) ?
It was not obvious to me what the macro was meant to iterate over.

Copy link
Member Author

Choose a reason for hiding this comment

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

T'was not obvious to me either so I went with T like in other txt-related macros and FOREACH like in other iterator macros.

TXT_FOREACH() was my alternative.

The pun was of course intended.

Copy link
Member

Choose a reason for hiding this comment

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

My useless contribution on bikeshed colors: TXT_FOREACH_CHAR
Please not TFOREACH, for consistency I'd otherwise propose Tforeach or Titer

Copy link
Member Author

Choose a reason for hiding this comment

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

I suppose Traverse() was the better name, too late...

Copy link
Member

Choose a reason for hiding this comment

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

Never too late to rename for the better ;)

@bsdphk
Copy link
Contributor

bsdphk commented Sep 23, 2024

I'm OK with any sensible name which is "in-family", ie T[a-z]*

Copy link
Member

@nigoroll nigoroll left a comment

Choose a reason for hiding this comment

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

You can merge this, phk agreed already

@dridi
Copy link
Member Author

dridi commented Sep 24, 2024

I was waiting for CI to complete after making the change.

@dridi dridi merged commit 3f3d701 into varnishcache:master Sep 24, 2024
10 of 11 checks passed
@dridi dridi deleted the tforeach branch September 24, 2024 07:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants