1. The way I've written the macro does appear to use a GNU extension
(namely, the ## token paste operator). This is necessary because of the
dangling comma in the call to printf when no arguments are given to the
variadic macro. While this is cleaner, we can get rid of the GNU extension
by creating two versions of the tracing macro: one that takes no arguments
and one that takes a variadic argument.
This does bring up another question I had: what standard are we coding
to for the C++ portion of the compiler? I would like to use some C++11
features in functionResolution.cpp, and this shouldn't impact its ability
to generate C99 code or for the generated code to link against a runtime
coded in C99.
2. From my understanding the 'inline' keyword does not ensure that the
compiler will inline a function. The only thing it requires the compiler
to do is change the way the function is linked inside the object file.
While GCC/Clang usually inline functions appropriately, if you REALLY want
to ensure it is inlined you can use the always_inline function attribute.
That being said, I don't see any harm in marking this as an inline function.
3. The way the language specification is written the only judgment about
a function that can be made during disambiguation is whether it is "more
specific" than another function or not. Therefore, the best you could hope
for (in the sense of reducing repeated code and better performance) is
something along the lines of:
if (isMoreSpecificMatch(fn1, fn2, ...) && !isMoreSpecificMatch(fn2, fn1,
...)) ...
Unfortunately, even this is impossible with the current specification.
Because the clauses for determining which function are more specific are
interleaved (see section 13.14.3 of the specification on page 106) you
won't get the same result if you do something like above. The reason for
this is that in the interleaved code the second clause may fire, indicating
that the second function is a better match, while in the first call to
isMoreSpecific in the non-interleaved code a later clause fires, indicating
that the first function is a better match. This later clause would never
have been reached in the interleaved code, and therefore we change the
fundamental behavior of the comparison.
Even more restrictive than that, changing the ordering of some of the
clauses can change the behavior of the comparison. The most extreme
example of this are clauses 8 and 9 in the argument mapping comparison
section (page 107). If the ordering of these clauses is changed in any way
the comparisons will yield very different results.
Something similar to the above example (and what you described) was what
we were originally going for. Unfortunately it is not possible without
rewriting the specification, which is something that Jeremy is looking into.
4. I'm certainly willing to refine the patch. I think adding
documentation is a great idea and we certainly need to figure out what to
do with the macros. Looking at the old and new versions side by side does
help though. I would also recommend looking at the new explain-call info
to see if you think it is clearer.
5. I would absolutely love to add documentation. Is there any objection
to me using Doxygen style comments so we could unleash Doxygen on the code
base at a later date?
Thanks for the feedback.
- Chris
On Wed, Nov 20, 2013 at 2:09 PM, Greg Titus <[email protected]> wrote:
> 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; chapel-developers@lists.
>> sourceforge.net
>> *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
