xkbcommon / libxkbcommon

keymap handling library for toolkits and window systems
https://xkbcommon.org
Other
286 stars 125 forks source link

Utils: steal `steal` from libei #407

Closed wismill closed 12 months ago

wismill commented 1 year ago

Follow-up of Peter’s suggestion.

whot commented 1 year ago

what's the compiler warning that is introduced here?

bluetech commented 1 year ago

I think we should only steal steal, since I doubt __attribute__((cleanup(_x))) works on msvc, and we don't use x*alloc functions. I'm also surprised __typeof__ works, but I see we're already using it in util-list and we didn't get any complaints, so it's OK.

wismill commented 1 year ago

quite disappointed you didn't say "steal steal from libei" but oh well :)

@whot Good one! Fixed 😉

what's the compiler warning that is introduced here?

Which one?

I think we should only steal steal

@bluetech I removed any function in util-mem.h that we do not use. Now I am not sure we should have a separate file with only one public macro.

@whot if we were to merge the now tiny util-mem.h or migrate memory-related function to it, should we keep the current license or would you agree to use license in e.g. utils.h and add you as author?

whot commented 1 year ago

what's the compiler warning that is introduced here?

Which one?

You added the compiler warning label to this MR, so I figured there may be a compiler warning :)

would you agree to use license in e.g. utils.h and add you as author?

the licenses are the same anyway but if there's a minor difference somewhere - yes, I agree to relicense to whatever specific wording libxkbcommon uses.

wismill commented 1 year ago

You added the compiler warning label to this MR, so I figured there may be a compiler warning :)

@whot Ah ok, now I understand. Well, there are some warnings, but they are unrelated. I chose this label because there is currently none for memory handling improvement. Now I see this is confusing.

About the new file util-mem.h: I think it makes sense to keep it and move the memory-related functions utils.h to util-mem.h (in another PR).

wismill commented 1 year ago

I added a new commit that use steal in xkbcomp files. I did not find other occurrences of the pattern:

a = b;
b = NULL;

but of course the simple regex I used cannot spot all the occurrences.

OTOH a recurrent pattern I saw is stealing a darray and re-initializing the source. We do have darray_steal, but it only steals the array, not the whole struct. I think it is worth another PR:

  1. Rename darray_steal to e.g. darray_steal_array
  2. Create a new macro darray_steal that steals the darray and re-initialize the source.
wismill commented 1 year ago

I'm also surprised __typeof__ works, but I see we're already using it in util-list and we didn't get any complaints, so it's OK.

@bluetech it does not work 😕 We did not catch this in the CI because it is only used in the registry, which is not built on Windows… Now that I use steal in xkbcomp, the CI fails. I am looking for a fix.

wismill commented 1 year ago

Removing __typeof__ for Windows works without warning. We loose some type safety, but is there another option other than switching to C23?

whot commented 1 year ago

We loose some type safety, but is there another option other than switching to C23?

IMO this doesn't matter too much here anyway because the type-safety still exists on other platforms so any issues we'll pick up there.

whot commented 1 year ago

but of course the simple regex I used cannot spot all the occurrences.

if you have too much time on your hand, you can learn coccinelle :)