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

[suyog sarda]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140306/5bc22a65/attachment.html>