|
Originally Posted by Legorol
I have just noticed this AddOn thanks to the publicity it received on the official forums. Good job, Iriel :-) It does look very nice and very useful. However, I would like to make a couple of small comments:
|
Thanks! I appreciate the feedback. I haven't read the WoW forums in a week or so, but the last I saw, Iriel was rather luke-warm on profiling. Can you point me to the post you saw?
|
1) In make_hook, you create a function that has ellipsis (...) as argument, so that it can receive multiple arguments. You then use unpack(arg) to pass these onto the hooked function. While this is very nice style-wise, the use of the ellipsis itself generates garbage, and therefore might bias your statistics.
|
I've already abandoned style for efficiency to handle return values. I originally used
local r = { info.orig_f(unpack(arg)) }
to pick up a dynamic number of function return values. This severely skewed the GC results though so I used the ugly and semi-unreliable code
local a,b,c,d,e = info.orig_f(unpack(arg))
to reduce garbage generation.
You really think it's a good idea to limit the number of arguments a function takes? I convinced myself that 5 return results is unlikely to break things, but I'm not sure what to use as the upper limit to the number of arguments. You think 20 is ok?
|
2) You are using the Class:Method syntax to enter functions to watch that are elements of a table. I suggest (and hope) that you use the Class.Method syntax instead. Although the choice is arbitrary from the entry point of view, it ties in better with the Lua concepts of elements of a table (Also see point 3). I know that usually the way to call most methods of XML objects is via the Class:Method syntax (e.g. GameTooltip:Show()), but in Lua, the colon is really nothing special, it's just a shorthand for Class.Method(Class, ...). In otherwords, "GameTooltip:Show() = GameTooltip.Show(GameTooltip)". Also, if you are trying to hook a function, you would specify GameTooltip.Show as the function to hook and not GameTooltip:Show.
|
There is another thread
http://www.wowinterface.com/forums/s...5&page=2&pp=10
where Kaelten asked for method call tracing on particular instances. After I thought about that, I realized I made the mistake you point out. I've decided to use "class.method" syntax to trace functions in a table (works for class methods and package namespaces) and "object:method" syntax to trace method calls on specific objects.
|
3) Do you think you will support functions that are more than one layer deep in a table? For example Class.Subclass.Function?
|
Honestly I didn't know people used that style. It's no big deal to add arbitrary nesting. I should also change the table search code to allow name patterns for tables. (In v1.3 only method names allow patterns; tables must be named precisely.)
|
4) I don't know if you noticed this, but unfortunately due to the way Blizzard coded things, you can't enumerate the native methods of tables that are representing XML-defined objects. You only have access to methods if you know their name exactly.
|
Thanks, I definitely wasn't aware of that.
Are you sure Bliz isn't using Lua metatables? I could automatically search __index if it's a table. If __index is a function or if Bliz is using non-Lua method dispatch then I'm pretty much hosed. I'm past the Lua-newbie phase, but I'm not an expert yet... If you have any advice on OO and/or metatables I'd love to hear it.
One of the things that I've sketched out for v1.4 is a more robust way of entering the functions to trace. People (especially gamers without Lua experience) are making mistakes entering the function name patterns. I'm planning on having a GUI to build the table and function patterns with check boxes to turn them on and off.
I could probably solve the "secret" method problem by including a built-in list of method names. (I'd rather follow __index up to the superclass if I can do that reliably though.)