On Wed, Oct 20, 2010 at 11:20 AM, Paul Berry <[email protected]> wrote:
> On Tue, Oct 19, 2010 at 7:59 PM, Markus Roberts <[email protected]>wrote: >> >>> >>> >>> >>>> And finally, I'm not seeing why the dodge with the context thread var is >>>>> needed. Isn't this just going to be the current AST node? >>>>> >>>>> >>>> >>>> Yes. But from lookupvar, we don't know what the current AST node is (as >>>> far as I could find). The goal is simply to find the current line and file, >>>> preferably from something that mixes in Util::Errors, so we can use >>>> error_context. If there's somewhere more correct to get that information >>>> from lookupvar, I would like to fix it, as it's a rather inelegant solution >>>> now. >>>> >>> >>> So two ideas: >>> >>> 1) The obvious idea would be to pass it in to lookupvar from the call >>> site (which should know the context), or if you'd rather think outside the >>> box >>> 2) conditionally raise an exception when you detect the error and move >>> the warning logic into safeevaluate, with a "retry" to resume after the >>> message is generated. >>> >> >> Regarding #1, lookupvar is called from a lot of places (ast evaluate >> methods, extlookup, Puppet::Parser::Resource, and template processing), and >> many of these don't have easy access to the current file and line number. >> >> #2 is dangerous because it involves retrying of code that potentially has >> side effects. To be certain that it was safe, we would have to trace >> through every code path from safeevaluate to lookupvar to make sure that it >> was safe to execute twice. >> >> Although I'm not generally a fan of thread-local variables, I think this >> is one situation where the advantages outweigh the disadvantages. I see two >> major advantages of the method we chose: 1. since the thread-local variable >> parallels safeevaluate's use of exception handling to tag error messages >> with file and line numbers, we have good reason to be confident that file >> and line number information will be applied to these warnings at least as >> reliably as it is applied to errors (and that is known to work well). 2. >> the method we chose will extend trivially to any warnings we may decide to >> add in the future. >> > So, ignoring the strawman behind door #2 for the moment, I would argue (fairly strongly) that calling lookupvar from all over the place is a real problem that should be fixed, especially if those calls are from places not easily tied to any particular AST node (in fact, you could make the case that, if these are too hard to tie to a particular place in the source the whole notion of "static" scoping for them is questionable). In reality the question to ask is shouldn't these three or four stray users really be talking to an AST::Variable instance rather than calling lookupvar? Further, this is the classic rational for introducing global variables; no one ever says "hey, it's to easy too determine this value at the call site; we should sneak it in with a global rather than passing it as a parameter." And it almost never ends well. As for the method extending trivially to future uses, that's a "we should sin now so it's easier to sin in the future" argument. If we really want to have this sort of error-source tracking mechanism it should be owned by something (probably the compiler or parser object) rather than being stuffed into a thread var, but the general trend has been away from that sort of stateful adjunct in general (and the compiler in specific) and towards making the process of evaluation purely functional. -- M ----------------------------------------------------------- The power of accurate observation is commonly called cynicism by those who have not got it. ~George Bernard Shaw ------------------------------------------------------------ -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
