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

[Richard Smith]

Committed as r203741. Thanks!


On Tue, Mar 11, 2014 at 8:48 PM, suyog sarda <sardask01 at gmail.com> wrote:

> 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/4fc875a7/attachment.html>