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
_______________________________________________ cfe-commits mailing list [email protected] http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
