Thanks a lot for the review. I am yet to get commit access. Can you please commit this patch for me. I will take a look into the formatting (test case formatting, comment wording, 80 column violations) from the committed patch itself for my future reference.
Thanks a lot once again. On Wed, Mar 12, 2014 at 3:32 AM, Richard Smith <[email protected]>wrote: > Sorry for the delay! > > Patch basically looks fine, though the testcase is in an inappropriate > file. I'd like to tweak a few tiny things (test case formatting, comment > wording, 80 column violations) -- if you need someone to commit this for > you, I'll make those tweaks and commit. If you have commit access yourself, > I'll give you some more feedback. Let me know! > > > On Mon, Mar 10, 2014 at 11:26 AM, suyog sarda <[email protected]> wrote: > >> Gentle Ping on this!! >> >> >> On Thu, Mar 6, 2014 at 11:24 AM, suyog sarda <[email protected]> wrote: >> >>> Gentle Ping. >>> >>> On Tue, Feb 25, 2014 at 11:27 PM, suyog sarda <[email protected]>wrote: >>> >>>> Hi, >>>> >>>> Sorry for late reply. Attaching updated patch with comments and test >>>> case. Please help in reviewing the same. >>>> >>>> >>>> On Sat, Feb 22, 2014 at 4:05 AM, Richard Smith >>>> <[email protected]>wrote: >>>> >>>>> >>>>> On Thu, Feb 20, 2014 at 8:24 AM, suyog sarda <[email protected]>wrote: >>>>> >>>>>> Hi, >>>>>> >>>>>> Thanks for the review of the patch. >>>>>> >>>>>> The test cases which i can think of involves templates are as follows >>>>>> : >>>>>> >>>>>> 1. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> *template <typename T> void f(const T); template <typename T> void >>>>>> f(T);template <typename T> void f(T x){ x = 0;}void f(){ >>>>>> f<int>(0);}* >>>>>> This won't throw any error, since there exist a declaration *template >>>>>> <typename T> void f(T)* which matches the instantiation. >>>>>> And i think we are treating both the declaration as separate >>>>>> declarations and not re-declaration, as explained in above mails from you >>>>>> and observed through code flow as well. >>>>>> >>>>>> 2. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> * template <typename T> void f(const T); template <typename T> void >>>>>> f(T x){ x = 0;}void f(){ f<int>(0);}* >>>>>> Even this won't throw any error as the declaration and definition are >>>>>> for separate functions. So basically, the first declaration doesn't have >>>>>> any function definition, and the function defined is a separate >>>>>> declaration >>>>>> itself, which matches the instantiation pattern. >>>>>> >>>>>> 3. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> *template <typename T> void f(const T);template <typename T> void >>>>>> f(const T x){ x = 0;}void f(){ f<int>(0); }* >>>>>> >>>>>> This will throw error since the definition itself has a const >>>>>> qualifier for the function variable and we are trying to modify the >>>>>> variable inside the function definition. This anyways was true even >>>>>> before >>>>>> applying patch. >>>>>> >>>>>> 4. >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> >>>>>> *template <typename T>struct A{ void f(const int);};template >>>>>> <typename T> void A<T>::f(int x){ x = 0;}void f(){ A<int> a; >>>>>> a.f(0);}* >>>>>> >>>>>> This test case doesn't have dependent function parameter. >>>>>> >>>>>> >>>>>> The focus of the solution for PR18275 got shifted to templates >>>>>> parameter checking rather than specific CV qualifier, which i completely >>>>>> agree as per your explanation above. And the test case listed in the bug >>>>>> gets inherently resolved with the patch. So, i am bit stuck as to how to >>>>>> form a test case for this patch (specific test case which will throw >>>>>> error >>>>>> because of this patch and it should obviously involve template as we are >>>>>> checking for dependent type). >>>>>> >>>>>> Can you please help in pointing out what specific should the test >>>>>> case contain? I am a starter for clang patches. Your help will be a lot >>>>>> more useful in learning things. >>>>>> >>>>> >>>>> Something like your case 4 above would be a good testcase. >>>>> >>>>> >>>>>> On Thu, Feb 20, 2014 at 2:30 AM, Richard Smith < >>>>>> [email protected]> wrote: >>>>>> >>>>>>> On Wed, Feb 19, 2014 at 12:52 PM, suyog sarda >>>>>>> <[email protected]>wrote: >>>>>>> >>>>>>>> Thanks Richard for the review. Even i felt that cv qualifier >>>>>>>> checking and setting is not suitable (though the standard 13.1/3 is >>>>>>>> specifically related to CV qualifiers). Earlier i was checking CV >>>>>>>> qualifiers and setting type, which obviously failed for function >>>>>>>> templates. >>>>>>>> And i agree to your point that if the parameter is not dependent then >>>>>>>> set >>>>>>>> the type as in instantiated function. >>>>>>>> >>>>>>>> In that case, do we still need to check if the parameters are CV >>>>>>>> qualified alongwith checking if parameter is dependent? I am attaching >>>>>>>> a >>>>>>>> patch where we do not check if the parameters are CV qualified but only >>>>>>>> checks if it is not dependent and then set the type. This patch works >>>>>>>> for >>>>>>>> the test case in the bug with no regression. Also, if we are not >>>>>>>> checking >>>>>>>> CV qualifiers and just checking only dependent type then is it >>>>>>>> feasible to >>>>>>>> put comment regarding standard 13.1/3? How should comment look if above >>>>>>>> patch is fine. >>>>>>>> >>>>>>> >>>>>>> The approach in the patch looks OK. I don't think referencing >>>>>>> 13.1/3 is worthwhile: it's reasonable to expect that anyone reading the >>>>>>> Clang source knows that parameter declarations in function >>>>>>> redeclarations >>>>>>> can differ in top-level cv-qualifiers. What's worth pointing out here is >>>>>>> (1) that we need the type to match the pattern we're instantiating, and >>>>>>> (2) >>>>>>> that we don't need to do this for dependent types because those >>>>>>> shouldn't >>>>>>> differ in cv-qualifiers (with a FIXME that we currently allow dependent >>>>>>> types with different cv-qualifeirs to be treated as redeclarations). >>>>>>> >>>>>>> >>>>>>>> Also, i am pasting test case from the bug itself. Please let me >>>>>>>> know which file to put it in. I cannot think of a negative test case >>>>>>>> for >>>>>>>> this bug. >>>>>>>> >>>>>>>> Your help is greatly appreciated. >>>>>>>> >>>>>>>> Test case : >>>>>>>> >>>>>>>> >>>>>>>> *struct B >>>>>>>> { >>>>>>>> void f(const int); >>>>>>>> }; >>>>>>>> >>>>>>>> void B::f(int x) >>>>>>>> { >>>>>>>> x = 0; >>>>>>>> } >>>>>>>> >>>>>>>> void f() >>>>>>>> { >>>>>>>> B b; >>>>>>>> b.f(0); >>>>>>>> }* >>>>>>>> >>>>>>>> This testcase doesn't look right: you need to use a template to >>>>>>> trigger the issue. >>>>>>> >>>>>>> >>>>>>>> On Wed, Feb 19, 2014 at 12:26 AM, Richard Smith >>>>>>>> <[email protected]>wrote: >>>>>>>> >>>>>>>>> This patch needs testcases. >>>>>>>>> >>>>>>>>> Also, the way in which you're updating the type of the parameter >>>>>>>>> doesn't look correct -- there's no reason to think that the >>>>>>>>> qualifiers will >>>>>>>>> be local. >>>>>>>>> >>>>>>>>> We discussed cases like this at the WG21 meeting in Issaquah last >>>>>>>>> week, and decided that cv-stripping should *not* be applied to >>>>>>>>> dependent >>>>>>>>> parameter types when determining whether two function templates are >>>>>>>>> redeclarations. Therefore: >>>>>>>>> >>>>>>>>> template<typename T> void f(T); >>>>>>>>> template<typename T> void f(const T); >>>>>>>>> >>>>>>>>> ... are not redeclarations (because, for instance, they have >>>>>>>>> different parameter types when T = int[]). >>>>>>>>> >>>>>>>>> That makes this problem easier to fix: if the parameter type >>>>>>>>> within PatternDecl is not dependent, set the type of the parameter in >>>>>>>>> the >>>>>>>>> instantiated function to that type. Otherwise, leave it alone, since >>>>>>>>> we >>>>>>>>> already know it will match. >>>>>>>>> >>>>>>>>> On Tue Feb 18 2014 at 6:02:55 AM, suyog sarda <[email protected]> >>>>>>>>> wrote: >>>>>>>>> >>>>>>>>>> Gentle Ping !! Please help in reviewing the patch for bug 18275. >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> -- >>>>>>>>>> With regards, >>>>>>>>>> Suyog Sarda >>>>>>>>>> >>>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> -- >>>>>>>> With regards, >>>>>>>> Suyog Sarda >>>>>>>> >>>>>>> >>>>>>> >>>>>> >>>>>> >>>>>> -- >>>>>> With regards, >>>>>> Suyog Sarda >>>>>> >>>>> >>>>> >>>> >>>> >>>> -- >>>> With regards, >>>> Suyog Sarda >>>> >>> >>> >>> >>> -- >>> With regards, >>> Suyog Sarda >>> >> >> >> >> -- >> With regards, >> Suyog Sarda >> > > -- With regards, Suyog Sarda
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
