On Tue, Oct 19, 2010 at 4:17 PM, Markus Roberts <[email protected]>wrote:
> > Puppet (master/apply) will now warn at most once per compile if the >> manifest being compiled uses dynamic scope. > > > Hmmm. At most once per compile? That still may be too frequent, and yet > I'm not sure off the top of my head how to reduce it. Also, does "at most" > mean it might not warn sometimes? If so, what's the rule? > The rule is zero times if there are no uses of dynamic scope, one otherwise. > > >> During variable lookup, >> it will perform both dynamic scope lookup and static scope lookup >> and compare the two. If they differ, it issues the warning. >> > > Any idea what the performance impact of the double-lookups is? And did you > explore detecting dynamic lookups rather than doing the double-lookup and > compare dodge (sitting on the sidelines, it looks like it might be a lot > less invasive -- just detect when you recurse into a parent where > self.parent_relationship == :dynamic, than warn if that's the case but you > haven't reached topscope when you find the var)? > > I haven't done any performance testing. But given that the lookup consists of a hash lookup and a recursive function call for each class being searched until the variable is found, each time a variable is looked up, I would expect the impact to be on the order of the amount of time it takes for the warning to print. The main reasoning behind this approach was that we only want to warn when there will be a behavior change when dynamic scope is removed. And so comparing the actual result we get with each leaves us with a high level of confidence that any warning will be a legitimate change. After thinking about it, though, I think checking parent_relationship as we recurse works just as well. Before implementing, it seemed as if there were complex cases in which we might get the wrong behavior, which made comparing the actual results the most appealing. But all the cases seem to boil down to the same thing. This approach would warn in the case where we get the same value from different sources, which won't technically be a behavior change, but maybe ought to be a warning anyway, since it is only incidentally correct. > 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. > -- 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]<puppet-dev%[email protected]> > . > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > -- 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.
