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

[Richard Smith]

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 <sardask01 at gmail.com> wrote:

> 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/20140311/ce274705/attachment.html>