Update string.lua #792

Merged
merged 1 commit into from Sep 9, 2014

Projects

None yet

6 participants

@Python1320
Contributor

No description provided.

@Lexicality
Contributor

Can I suggest more descriptive commit messages? "Small optimisations to string.lua" allows you to know what at least vaguely what the update was about without checking the changes.

@AshleighAdams
Contributor

@Lexicality I don't even think many of these kind of premature optimizations should be done either. The change at 279 I guess is fine, but making locals to the global variables is not worth the very little speed increase (if any), considering it could potentially introduce bugs and complicates code.

@willox
Collaborator
willox commented Aug 23, 2014

^ Exactly this. The change in performance is so tiny nowadays with LuaJIT that it isn't worth the time to localise libraries.

@Python1320
Contributor

The reasoning behind this small change is that string = "hi" doesn't cause stack overflows and mask actual errors that could tell you someone just broke the _G.string reference. Besides, do you really want string's metatable consult _G.string.

JIT can optimize only so much and afaik localizing global tables is more useful than not optimizing it: http://wiki.luajit.org/Numerical-Computing-Performance-Guide
It gets more complex than that though: http://lua-users.org/lists/lua-l/2011-02/msg01284.html.

TLDR: This wasn't about optimization except that one line. This was about causing less breakage when someone does dumb stuff.

@willox
Collaborator
willox commented Aug 23, 2014

The reasoning behind this small change is that string = "hi" doesn't cause stack overflows and mask actual errors that could tell you someone just broke the _G.string reference.

I hadn't thought of that, that's hilarious and probably happens in at least one badly coded addon.

@neico
Contributor
neico commented Aug 23, 2014

What you should think about tough is:

If your script happens to run after said badly coded addon (which can very well happen due to the unsorted nature of calling it), will local string = string contain the string table, or "hi"?

It's a basic thought that imo nullifies those local declarations being used at all...

@Python1320
Contributor

Extensions load before anything unless you've overriding files, look at init.lua.
This is to prevent things going totally broke when you do _G.string="hi", which causes massive recursion problems and spams and just masks the problem. With this addons are going to complain about attempt to index a string value if string is .... set into a string.

@neico
Contributor
neico commented Aug 23, 2014

Right, was more refering to other addons in that case (as I've seen addons using it as well for some reason here and there)

Tough using the method syntax (colon) instead of "string." could avoid that as well since even if you override _G.string the metatable for strings is still intact (I still don't know why the code is so inconsistent with that, string.lua is a good example where "string." as well as ":" is being used side by side)

also, if you want to start with that local stuff, then do it fully:

local table = table
local pairs = pairs
local tonumber = tonumber
local getmetatable = getmetatable
local error = error
local type = type
local Color = Color
local tostring = tostring
@AshleighAdams
Contributor

If someone fucks up the string table, everything is going to break.

What about if someone–for whatever reason–replaces the string table, rather than updating the stuff inside of it? If that happens, this won't use the new table, and will continue using the old one, which is undesired behavior; whether it makes sense to do such or not is irrelevant.

@Lexicality
Contributor

@Python1320

TLDR: This wasn't about optimization except that one line. This was about causing less breakage when someone does dumb stuff.

This is why you use descriptive commit messages! :eng101:

@KateAdams Anyone who replaces global libraries and expects the rest of the environment to use the new global is an idiot (since it won't - modules localise too), so let's not bother to consider if this might upset them.

@robotboy655 robotboy655 merged commit 2ae0f13 into garrynewman:master Sep 9, 2014
@Python1320 Python1320 deleted the Python1320:patch-3 branch Sep 9, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment