Committed as r203741. Thanks!
On Tue, Mar 11, 2014 at 8:48 PM, suyog sarda <[email protected]> wrote: > 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
