Variadic macros are standard C99, but gcc supports them with slightly different syntax in order to give some additional capabilities, so to maintain portability one has to be careful about how they're written. I haven't looked in detail at the proposed code, so I can't comment as to whether this would pose an issue for the usage there.
greg On 11/20/2013 12:46 PM, Tom Hildebrandt wrote: > Hi Chris: > > 1. Variadic macros appear to be a GCC extension. Our base language for backend > support is C99, so more recent additions to the language cannot be used. This > is to maintain backend portability. > > 2. I like the conversion of registerParamPreference into a static function. It > could be marked "inline" to more closely mimic its definition as a macro. > However, GCC is smart enough to figure out whether it can be benefically > inlined, even without the hint. > > 3. I was hoping to see a solution that roughly boiled down to: > score1 = scoreMatch(candidateFn1, info.actuals, actualFormals, scope, ...); > score2 = scoreMatch(candidateFn2, info.actuals, actualFormals, scope, ...); > if (score1 < score2) worse = true; > if (score1 == score2) equal = true; > > // Same loop logic > > One could argue that there is some performance gain due to early exit when the > first candidate is found to be worse. But I doubt it. Integer or boolean > arithmetic is at least 10 times faster than chained decisions. So replacing > all those "if"s with a simple algebraic expression to compute a score is > likely to be a net win for performance. > > In any case, it is axiomatic that "repeated code" and "early optimization" are > bug farms, so finding a solution that avoids both will be a boon. > > Another possible implementation is to carry along the set of candidates, and > apply successively more restrictive filters. If at any point the set of > candidates contains exactly one member, a match has been found. If it contains > zero, then the result is ambiguity up to the application of the current > filter. (For error reporting, one would maintain a copy of the previous > candidate set, so the conflicting candidates could be listed.) > > 4. That said, I think your factoring still represents an improvement. So if > you don't feel inclined to rewrite it, we can proceed. Let me know if you > stand behind this patch, so I can apply it and give it a more detailed review > it "in situ". I have been looking only at the patch file, and might appreciate > it more by viewing the old and new side by side. > > 5. In any case, it would be helpful if you added a brief header comment for > each new class you have introduced, describing what it represents and how it > is used. > > Please respond, > Tom H. > > > ------------------------------------------------------------------------------ > *From:* Tom Hildebrandt [[email protected]] > *Sent:* Tuesday, November 19, 2013 10:20 PM > *To:* Brad Chamberlain; Chris Wailes; [email protected] > *Subject:* Re: [Chapel-developers] Request for Review: Refactoring > Disambiguation By Match > > All: > I have considered rewriting this function, so I may have some useful insights. > I will review it and post my comments tomorrow. > Tom H. > ------------------------------------------------------------------------------ > *From:* Brad Chamberlain [[email protected]] > *Sent:* Tuesday, November 19, 2013 9:12 PM > *To:* Chris Wailes; [email protected] > *Subject:* Re: [Chapel-developers] Request for Review: Refactoring > Disambiguation By Match > > I've been working in this part of the code a lot lately (relatively to the > rest of the group), and so would be a logical reviewer, and am definitely > interested in reviewing it, but am not certain that I'll have time to before > the end of Thanksgiving week (and then will have some digging out from backlog > to do). If someone else from the core team wants to review it in the meantime, > I can review it post-commit after I get back; or if Chris can wait, I can do > it then. > > -Brad > > > ------------------------------------------------------------------------------ > *From:* Chris Wailes [[email protected]] > *Sent:* Tuesday, November 19, 2013 3:50 PM > *To:* [email protected] > *Subject:* [Chapel-developers] Request for Review: Refactoring Disambiguation > By Match > > This patch is a refactoring of the disambiguation_by_match function. It cleans > up the function interface, splits the functionality into several > easier-to-understand functions, and renames variables (and rearranges some > Boolean conditionals to have the naming make sense) to make their function > clearer. In addition, information from comparisons is recorded and reused to > skip over functions that can't be the best match. Lastly, the tracing enabled > by --explain-call has been improved. > > - Chris > > > ------------------------------------------------------------------------------ > Shape the Mobile Experience: Free Subscription > Software experts and developers: Be at the forefront of tech innovation. > Intel(R) Software Adrenaline delivers strategic insight and game-changing > conversations that shape the rapidly evolving mobile landscape. Sign up now. > http://pubads.g.doubleclick.net/gampad/clk?id=63431311&iu=/4140/ostg.clktrk > > > > _______________________________________________ > Chapel-developers mailing list > [email protected] > https://lists.sourceforge.net/lists/listinfo/chapel-developers ------------------------------------------------------------------------------ Shape the Mobile Experience: Free Subscription Software experts and developers: Be at the forefront of tech innovation. Intel(R) Software Adrenaline delivers strategic insight and game-changing conversations that shape the rapidly evolving mobile landscape. Sign up now. http://pubads.g.doubleclick.net/gampad/clk?id=63431311&iu=/4140/ostg.clktrk _______________________________________________ Chapel-developers mailing list [email protected] https://lists.sourceforge.net/lists/listinfo/chapel-developers
