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

[Richard Smith]

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