I would be happy with the documentation going in in *any* format. I suggest committing once the technical issues have been resolved (which it sounds like they are), given that this is quite time in terms of commits. We can - and I am suggesting to - address the doxygen issue separately.
Vass On 11/20/2013 01:10 PM, Chris Wailes wrote: > That's fine with me. I've fixed the macro issue and added the inline > keyword, mainly to help document the intent of the code. I'll add the > documentation when a format is decided on. > > - Chris > > > On Wed, Nov 20, 2013 at 3:50 PM, Tom Hildebrandt<[email protected]> wrote: > >> Hi Chris: >> >> Thanks for your response. >> >> 1. You are right about the C++ compiler not affecting the C output. I >> was confusing the compiler source with generated code. >> >> Apparently, C++ support for variadic macros came after the C++99 update. >> For the C++ compilers, I think we rely more on the behavior of the set of >> platforms we support than a specific C++ version. Our standard >> configuration (64-bit SUSE Enterprise) uses g++ 4.3.4, which predates the >> C++0x11 standard, and other supported platforms are of an older lineage. >> So I don't think you can expect C++0x11 features to be supported >> uniformly. It's probably best to stay away from them for the time being. >> >> 2. I was just noting a possible difference between a macro vs. static >> implementations of the code. Since my expectation is that g++ will inline >> appropriately even if "inline" is not explicitly specified, there is no >> harm in leaving it off. >> >> 3. I think that that area of the specification (regarding function >> disambiguation) deserves some review. So I will offer to update the >> specification (in collaboration with Jeremy). >> >> 4. Before you spend much time updating the patch, let me get feedback on >> these two open issues (3 and 5). >> >> 5. I will raise the issue of whether we want to use (or at least accept) >> Doxygen for internal documentation of the compiler code. >> >> Many of the team members are attending the SuperComputing conference >> this week, so it may be another week or so before these issues are >> resolved. I hope that is acceptable to you. >> >> Tom H. >> >> ------------------------------ >> *From:* Chris Wailes [[email protected]] >> *Sent:* Wednesday, November 20, 2013 12:19 PM >> *To:* Greg Titus >> *Cc:* Tom Hildebrandt; Brad Chamberlain; >> [email protected] >> *Subject:* Re: [Chapel-developers] Request for Review: Refactoring >> Disambiguation By Match >> >> >> 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 ------------------------------------------------------------------------------ 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
