Check C++ Bad Override

classic Classic list List threaded Threaded
8 messages Options
Reply | Threaded
Open this post in threaded view
|

Check C++ Bad Override

Lei Zhang
Hi Clang,

The attached patch adds a C++ Checker to clang, it checks whether
there are mistakes in override.

The mistake mean things like this:

class base {
  virtual void foo() const {/*...*/}
};

class child: public base {
  /* child::foo is probably meant to override base::foo but type
signatures don't match
  perfectly */
  void foo() {}
};

and this:

class base {
  void foo() const {}
};

class child: public base {
  /* child::foo is probably meant to override base::foo but
 that function is not virtual. */
  void foo() const {}
};

I implement this as a static analysis checker, because i think it's
not too visible and not too fast.

I will appreciate it if there are any advice about this patch.

--
Best regards!

Lei Zhang

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev

OverrideChecker.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Check C++ Bad Override

Nico Weber
Hi,

On Fri, Oct 7, 2011 at 7:12 AM, 章磊 <[hidden email]> wrote:

> Hi Clang,
>
> The attached patch adds a C++ Checker to clang, it checks whether
> there are mistakes in override.
>
> The mistake mean things like this:
>
> class base {
>  virtual void foo() const {/*...*/}
> };
>
> class child: public base {
>  /* child::foo is probably meant to override base::foo but type
> signatures don't match
>  perfectly */
>  void foo() {}
> };

Isn't this caught by -Woverloaded-virtual?

> and this:
>
> class base {
>  void foo() const {}
> };
>
> class child: public base {
>  /* child::foo is probably meant to override base::foo but
>  that function is not virtual. */
>  void foo() const {}
> };

hans prototyped something like this at
http://llvm.org/bugs/show_bug.cgi?id=10234 , but it seemed to noisy to
be useful. (I haven't read how your patch does this though).

Nico

>
> I implement this as a static analysis checker, because i think it's
> not too visible and not too fast.
>
> I will appreciate it if there are any advice about this patch.
>
> --
> Best regards!
>
> Lei Zhang
>
> _______________________________________________
> cfe-dev mailing list
> [hidden email]
> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>
>

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Check C++ Bad Override

Don Quixote de la Mancha
On Fri, Oct 7, 2011 at 7:33 AM, Nico Weber <[hidden email]> wrote:
>> The attached patch adds a C++ Checker to clang, it checks whether
>> there are mistakes in override.

My understanding is that it is not actually necessary to declare
member functions in subclasses as being virtual if the base class
declares them virtual.  The compiler just assumes they are.

That's my understanding anyway.  I do not own a copy of the C++ Spec
so I can't Quote Scripture.

I always declare them virtual anyway because it's confusing otherwise.
 Is that what you're trying to catch?




--
Don Quixote de la Mancha
Dulcinea Technologies Corporation
Software of Elegance and Beauty
http://www.dulcineatech.com
[hidden email]
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Check C++ Bad Override

Arjun Singri
If you see the signatures of foo in the base class and the derived class, you can see that the derived class version is missing the "const" keyword.

On Fri, Oct 7, 2011 at 5:12 PM, Don Quixote de la Mancha <[hidden email]> wrote:
On Fri, Oct 7, 2011 at 7:33 AM, Nico Weber <[hidden email]> wrote:
>> The attached patch adds a C++ Checker to clang, it checks whether
>> there are mistakes in override.

My understanding is that it is not actually necessary to declare
member functions in subclasses as being virtual if the base class
declares them virtual.  The compiler just assumes they are.

That's my understanding anyway.  I do not own a copy of the C++ Spec
so I can't Quote Scripture.

I always declare them virtual anyway because it's confusing otherwise.
 Is that what you're trying to catch?




--
Don Quixote de la Mancha
Dulcinea Technologies Corporation
Software of Elegance and Beauty
http://www.dulcineatech.com
[hidden email]
_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Check C++ Bad Override

Arjun Singri
In reply to this post by Lei Zhang
Have you run this on any open source software to see how many results/violations show up? If yes, sharing the results will be useful. If not, then its a good exercise to run it on an open source software. You can even try running it on llvm itself.

Arjun

On Fri, Oct 7, 2011 at 7:12 AM, 章磊 <[hidden email]> wrote:
Hi Clang,

The attached patch adds a C++ Checker to clang, it checks whether
there are mistakes in override.

The mistake mean things like this:

class base {
 virtual void foo() const {/*...*/}
};

class child: public base {
 /* child::foo is probably meant to override base::foo but type
signatures don't match
 perfectly */
 void foo() {}
};

and this:

class base {
 void foo() const {}
};

class child: public base {
 /* child::foo is probably meant to override base::foo but
 that function is not virtual. */
 void foo() const {}
};

I implement this as a static analysis checker, because i think it's
not too visible and not too fast.

I will appreciate it if there are any advice about this patch.

--
Best regards!

Lei Zhang

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev



_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Check C++ Bad Override

Lei Zhang
In reply to this post by Nico Weber
2011/10/7, Nico Weber <[hidden email]>:

> Hi,
>
> On Fri, Oct 7, 2011 at 7:12 AM, 章磊 <[hidden email]> wrote:
>> Hi Clang,
>>
>> The attached patch adds a C++ Checker to clang, it checks whether
>> there are mistakes in override.
>>
>> The mistake mean things like this:
>>
>> class base {
>>  virtual void foo() const {/*...*/}
>> };
>>
>> class child: public base {
>>  /* child::foo is probably meant to override base::foo but type
>> signatures don't match
>>  perfectly */
>>  void foo() {}
>> };
>
> Isn't this caught by -Woverloaded-virtual?
Yes, it is caught by -Woverloaded-virtual. So it seems no need to
implement it as a static analysis checker. I will remove relevant code
from my patch.

>
>> and this:
>>
>> class base {
>>  void foo() const {}
>> };
>>
>> class child: public base {
>>  /* child::foo is probably meant to override base::foo but
>>  that function is not virtual. */
>>  void foo() const {}
>> };
>
> hans prototyped something like this at
> http://llvm.org/bugs/show_bug.cgi?id=10234 , but it seemed to noisy to
> be useful. (I haven't read how your patch does this though).
thanks, i will see test my patch on some open source code. And also
read hans's patch to see what i can do in static analysis.

>
> Nico
>
>>
>> I implement this as a static analysis checker, because i think it's
>> not too visible and not too fast.
>>
>> I will appreciate it if there are any advice about this patch.
>>
>> --
>> Best regards!
>>
>> Lei Zhang
>>
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>
>>
>

--
Best regards!

Lei Zhang


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Check C++ Bad Override

Lei Zhang
In reply to this post by Arjun Singri
Hi Arjun,

I have not run this patch on some open source software, but i will do
it later. When i got any results, i will let you know asap.

2011/10/8, Arjun Singri <[hidden email]>:

> Have you run this on any open source software to see how many
> results/violations show up? If yes, sharing the results will be useful. If
> not, then its a good exercise to run it on an open source software. You can
> even try running it on llvm itself.
>
> Arjun
>
> On Fri, Oct 7, 2011 at 7:12 AM, 章磊 <[hidden email]> wrote:
>
>> Hi Clang,
>>
>> The attached patch adds a C++ Checker to clang, it checks whether
>> there are mistakes in override.
>>
>> The mistake mean things like this:
>>
>> class base {
>>  virtual void foo() const {/*...*/}
>> };
>>
>> class child: public base {
>>  /* child::foo is probably meant to override base::foo but type
>> signatures don't match
>>  perfectly */
>>  void foo() {}
>> };
>>
>> and this:
>>
>> class base {
>>  void foo() const {}
>> };
>>
>> class child: public base {
>>  /* child::foo is probably meant to override base::foo but
>>  that function is not virtual. */
>>  void foo() const {}
>> };
>>
>> I implement this as a static analysis checker, because i think it's
>> not too visible and not too fast.
>>
>> I will appreciate it if there are any advice about this patch.
>>
>> --
>> Best regards!
>>
>> Lei Zhang
>>
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>
>>
>

--
Best regards!

Lei Zhang


_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
Reply | Threaded
Open this post in threaded view
|

Re: Check C++ Bad Override

Lei Zhang
Hi Arjun,
 
I tried this patch on llvm itself, it's too noisy.
 
I reviewed some of them and i think it's not the right way to implement this.
 
I think i have to cancle it.

在 2011年10月8日 下午1:06,章磊 <[hidden email]>写道:
Hi Arjun,

I have not run this patch on some open source software, but i will do
it later. When i got any results, i will let you know asap.

2011/10/8, Arjun Singri <[hidden email]>:
> Have you run this on any open source software to see how many
> results/violations show up? If yes, sharing the results will be useful. If
> not, then its a good exercise to run it on an open source software. You can
> even try running it on llvm itself.
>
> Arjun
>
> On Fri, Oct 7, 2011 at 7:12 AM, 章磊 <[hidden email]> wrote:
>
>> Hi Clang,
>>
>> The attached patch adds a C++ Checker to clang, it checks whether
>> there are mistakes in override.
>>
>> The mistake mean things like this:
>>
>> class base {
>>  virtual void foo() const {/*...*/}
>> };
>>
>> class child: public base {
>>  /* child::foo is probably meant to override base::foo but type
>> signatures don't match
>>  perfectly */

>>  void foo() {}
>> };
>>
>> and this:
>>
>> class base {
>>  void foo() const {}
>> };
>>
>> class child: public base {
>>  /* child::foo is probably meant to override base::foo but
>>  that function is not virtual. */
>>  void foo() const {}
>> };
>>
>> I implement this as a static analysis checker, because i think it's
>> not too visible and not too fast.
>>
>> I will appreciate it if there are any advice about this patch.
>>
>> --
>> Best regards!
>>
>> Lei Zhang
>>
>> _______________________________________________
>> cfe-dev mailing list
>> [hidden email]
>> http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev
>>
>>
>


--
Best regards!

Lei Zhang



--
Best regards!

Lei Zhang

_______________________________________________
cfe-dev mailing list
[hidden email]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-dev