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

[suyog sarda]

Thanks a lot for the review.

I am yet to get commit access. Can you please commit this patch for me. I
will take a look into the formatting (test case formatting, comment
wording, 80 column violations) from the committed patch itself for my
future reference.

Thanks a lot once again.


On Wed, Mar 12, 2014 at 3:32 AM, Richard Smith <richard at metafoo.co.uk>wrote:

> 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
>>
>
>


-- 
With regards,
Suyog Sarda
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/cfe-commits/attachments/20140312/96f27750/attachment.html>