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

refactor(core): add an empty table in constants then we can reuse it #11443

Closed
wants to merge 6 commits into from

Conversation

chronolaw
Copy link
Contributor

@chronolaw chronolaw commented Aug 23, 2023

Summary

The read-only empty table is very useful, it is widely used in kong's code base,
but its definition scatters in many different files.

This PR defines one and only empty table in constants.lua, other lua modules can use it easily.

I just changed EMPTY in some but not all modules,
if most of us think it is good, we will try to change all of them.

Checklist

Full changelog

  • [Implement ...]

Issue reference

Fix #[issue number]

@chronolaw chronolaw added the pr/discussion This PR is being debated. Probably just a few details. label Aug 23, 2023
@chronolaw chronolaw marked this pull request as ready for review August 23, 2023 09:22
@bungle
Copy link
Member

bungle commented Aug 24, 2023

Is the EMPTY most of the time used in (to avoid if statements or something):

local x = (d or EMPTY).x or "default"
local x = (d and d.doge or EMPTY).x or "default"

Which you also see in forms of:

local x = (d or {}).x or "default"
local x = (d and d.doge or {}).x or "default"

I am not huge fan of either, but some prefer it. The constant seems to be just to avoid table creation (but I am not sure if in those above examples LuaJIT would optimize it away anyway, and even if not, is it a big deal). The later example has benefit of not requiring a module to get EMPTY and is also shorter, and doesn't leak.

If I were to pick, I would just change code everywhere to just or {}. Without that EMPTY thing in constants or module level global.

@hanshuebner
Copy link
Contributor

I don't see the benefit of an EMPTY constant. It is longer than {} when used and requires an additional declaration. It also reminds me of #define BEGIN {. Let's not do this.

@chronolaw
Copy link
Contributor Author

We already defined many EMPTY in modules, could this PR reduce the duplicated code?

@bungle
Copy link
Member

bungle commented Aug 24, 2023

We already defined many EMPTY in modules, could this PR reduce the duplicated code?

I guess the suggestion was to remove all EMPTYs everywhere and just or {}.

@chronolaw
Copy link
Contributor Author

Let's keep the code base no changing until we get a agreement.

@chronolaw chronolaw closed this Aug 24, 2023
@chronolaw chronolaw deleted the refactor/add_empty_table_in_constants branch August 24, 2023 09:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
core/balancer core/clustering core/configuration pr/discussion This PR is being debated. Probably just a few details. size/S
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants