Convert the no-inline-virtuals rule into a constructors rule.

Convert the no-inline-virtuals rule into a constructors rule.

Inline virtual functions are now allowed, but if a class has too many of
them, it needs an out-of-line constructor.

This was discussed in
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/rdF1mT3V_PM/discussion.

This change currently includes base classes in counting inline virtuals, but that'll make it easier to break derived classes by changing their bases. If folks are annoyed, we could revert that part (change getFinalOverriders into methods()), at the cost of reducing the accuracy of the check.


CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
BUG=565523

[Jeffrey Yasskin, 2015-12-03 02:41:10]

Description was changed from

==========
Convert the no-inline-virtuals rule into a constructors rule.

Inline virtual functions are now allowed, but if a class has too many of
them, it needs an out-of-line constructor.

This was discussed in
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/rdF1mT3V_PM/dis....


Also start fixing up some of the new warnings.

I'm not sure I should actually be checking base classes, since that makes it
easy to break code far away from a change to a base class.
==========

to

==========
Convert the no-inline-virtuals rule into a constructors rule.

Inline virtual functions are now allowed, but if a class has too many of
them, it needs an out-of-line constructor.

This was discussed in
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/rdF1mT3V_PM/dis....


Also start fixing up some of the new warnings.

I'm not sure I should actually be checking base classes, since that makes it
easy to break code far away from a change to a base class.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Jeffrey Yasskin, 2015-12-03 02:42:02]

Description was changed from

==========
Convert the no-inline-virtuals rule into a constructors rule.

Inline virtual functions are now allowed, but if a class has too many of
them, it needs an out-of-line constructor.

This was discussed in
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/rdF1mT3V_PM/dis....


Also start fixing up some of the new warnings.

I'm not sure I should actually be checking base classes, since that makes it
easy to break code far away from a change to a base class.
CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Convert the no-inline-virtuals rule into a constructors rule.

Inline virtual functions are now allowed, but if a class has too many of
them, it needs an out-of-line constructor.

This was discussed in
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/rdF1mT3V_PM/dis....


Also start fixing up some of the new warnings.

I'm not sure I should actually be checking base classes, since that makes it
easy to break code far away from a change to a base class.


CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Jeffrey Yasskin, 2015-12-03 02:43:56]

Description was changed from

==========
Convert the no-inline-virtuals rule into a constructors rule.

Inline virtual functions are now allowed, but if a class has too many of
them, it needs an out-of-line constructor.

This was discussed in
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/rdF1mT3V_PM/dis....


Also start fixing up some of the new warnings.

I'm not sure I should actually be checking base classes, since that makes it
easy to break code far away from a change to a base class.


CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Convert the no-inline-virtuals rule into a constructors rule.

Inline virtual functions are now allowed, but if a class has too many of
them, it needs an out-of-line constructor.

This was discussed in
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/rdF1mT3V_PM/dis....


Also start fixing up some of the new warnings.

TODO:
 * I'm not sure I should actually be checking base classes, since that makes it
easy to break code far away from a change to a base class.
 * Add the flag
https://chromium.googlesource.com/chromium/src/+/master/docs/writing_clang_pl...
asks for.
 * Split up into more reviewable changes.


CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

[Jeffrey Yasskin, 2015-12-03 20:00:35]

Description was changed from

==========
Convert the no-inline-virtuals rule into a constructors rule.

Inline virtual functions are now allowed, but if a class has too many of
them, it needs an out-of-line constructor.

This was discussed in
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/rdF1mT3V_PM/dis....


Also start fixing up some of the new warnings.

TODO:
 * I'm not sure I should actually be checking base classes, since that makes it
easy to break code far away from a change to a base class.
 * Add the flag
https://chromium.googlesource.com/chromium/src/+/master/docs/writing_clang_pl...
asks for.
 * Split up into more reviewable changes.


CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
==========

to

==========
Convert the no-inline-virtuals rule into a constructors rule.

Inline virtual functions are now allowed, but if a class has too many of
them, it needs an out-of-line constructor.

This was discussed in
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/rdF1mT3V_PM/dis....


Also start fixing up some of the new warnings.

TODO:
 * I'm not sure I should actually be checking base classes, since that makes it
easy to break code far away from a change to a base class.
 * Add the flag
https://chromium.googlesource.com/chromium/src/+/master/docs/writing_clang_pl...
asks for.
 * Split up into more reviewable changes.


CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
BUG=565523
==========

[Jeffrey Yasskin, 2015-12-04 02:06:46]

Description was changed from

==========
Convert the no-inline-virtuals rule into a constructors rule.

Inline virtual functions are now allowed, but if a class has too many of
them, it needs an out-of-line constructor.

This was discussed in
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/rdF1mT3V_PM/dis....


Also start fixing up some of the new warnings.

TODO:
 * I'm not sure I should actually be checking base classes, since that makes it
easy to break code far away from a change to a base class.
 * Add the flag
https://chromium.googlesource.com/chromium/src/+/master/docs/writing_clang_pl...
asks for.
 * Split up into more reviewable changes.


CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
BUG=565523
==========

to

==========
Convert the no-inline-virtuals rule into a constructors rule.

Inline virtual functions are now allowed, but if a class has too many of
them, it needs an out-of-line constructor.

This was discussed in
https://groups.google.com/a/chromium.org/d/topic/chromium-dev/rdF1mT3V_PM/dis....

This change currently includes base classes in counting inline virtuals, but
that'll make it easier to break derived classes by changing their bases. If
folks are annoyed, we could revert that part (change getFinalOverriders into
methods()), at the cost of reducing the accuracy of the check.


CQ_INCLUDE_TRYBOTS=tryserver.blink:linux_blink_rel
BUG=565523
==========

[Jeffrey Yasskin, 2015-12-04 03:37:39]

jyasskin@chromium.org changed reviewers:
+ thakis@chromium.org

[Jeffrey Yasskin, 2015-12-04 03:37:39]

https://codereview.chromium.org/1499003003 enables the flag.
https://codereview.chromium.org/1499793003 shows some of the code that needs
fixing.

This currently doesn't set a hard limit on the size of inline virtuals. Let me
know if you want me to do that.

[Jeffrey Yasskin, 2015-12-16 22:22:28]

PTAL. If you accept this, I'll also change the Chromium style guide and inlining
guide to match, as outlined in
https://groups.google.com/a/chromium.org/d/msg/chromium-dev/rdF1mT3V_PM/Xxxri....

[danakj, 2016-03-11 21:36:56]

danakj@chromium.org changed reviewers:
+ danakj@chromium.org

[danakj, 2016-03-11 21:36:57]

proxy ping :)

[danakj, 2016-03-11 21:37:16]

danakj@chromium.org changed reviewers:
- danakj@chromium.org

[danakj, 2016-03-14 21:22:08]

danakj@chromium.org changed reviewers:
+ dcheng@chromium.org

[danakj, 2016-03-14 21:22:09]

Maybe Daniel can review?

[dcheng, 2016-03-24 17:37:25]

https://codereview.chromium.org/1493813003/diff/100001/tools/clang/plugins/Fi...
File tools/clang/plugins/FindBadConstructsConsumer.cpp (right):

https://codereview.chromium.org/1493813003/diff/100001/tools/clang/plugins/Fi...
tools/clang/plugins/FindBadConstructsConsumer.cpp:104: bool IsLeafClass(const
CXXRecordDecl& record) {
How accurate is this heuristic for determining leafiness? What were the results
when you built Chromium with your plugin changes active?

https://codereview.chromium.org/1493813003/diff/100001/tools/clang/plugins/Fi...
tools/clang/plugins/FindBadConstructsConsumer.cpp:328: for (const
std::pair<const CXXMethodDecl*, OverridingMethods>& method :
Another way to write this might be to iterate over CXXRecordDecl::methods():
would doing that instead work here? I don't think we'd need the deduping set in
that case, and we can use CXXMethodDecl::size_overridden_methods() to determine
if it's an override or not.

https://codereview.chromium.org/1493813003/diff/100001/tools/clang/plugins/te...
File tools/clang/plugins/tests/test.sh (right):

https://codereview.chromium.org/1493813003/diff/100001/tools/clang/plugins/te...
tools/clang/plugins/tests/test.sh:46: flags="${flags} -Xclang
-plugin-arg-find-bad-constructs \
You might need to rebase this, since I ported the tests over to Python for
Windows coverage.

https://codereview.chromium.org/1493813003/diff/100001/tools/clang/plugins/te...
File tools/clang/plugins/tests/virtual_bodies.h (right):

https://codereview.chromium.org/1493813003/diff/100001/tools/clang/plugins/te...
tools/clang/plugins/tests/virtual_bodies.h:47: // Should not warn if enough
virtual methods are out-of-line.
Nit: I find this comment a bit confusing because the check is actually
implemented as # of inline virtuals > some complexity limit, and not (directly)
a count of the number of out-of-lined virtuals.

https://codereview.chromium.org/1493813003/diff/100001/tools/clang/plugins/te...
tools/clang/plugins/tests/virtual_bodies.h:92: // Complain about the inline
destructor.
Nit: non-virtual inline