[cfe-dev] Bug 18275 - Incorrect const qualifier behavior in definition.

[suyog sarda]

Gentle Ping on this!!


On Thu, Mar 6, 2014 at 11:24 AM, suyog sarda <sardask01 at gmail.com> wrote:

> Gentle Ping.
>
> On Tue, Feb 25, 2014 at 11:27 PM, suyog sarda <sardask01 at gmail.com> 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 <richard at metafoo.co.uk>wrote:
>>
>>>
>>> On Thu, Feb 20, 2014 at 8:24 AM, suyog sarda <sardask01 at gmail.com>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 <richard at metafoo.co.uk>wrote:
>>>>
>>>>> On Wed, Feb 19, 2014 at 12:52 PM, suyog sarda <sardask01 at gmail.com>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 <metafoo at gmail.com>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 <sardask01 at gmail.com>
>>>>>>> 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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140310/a0e1c441/attachment.html>