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

[suyog sarda]

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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140225/1fd55f1d/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: PR_18275.patch
Type: text/x-patch
Size: 2214 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140225/1fd55f1d/attachment.bin>