1756504 - Remove cpp-virtual-final linter, a minor style check that doesn't diagnose real bugs.

Status: RESOLVED FIXED

(Core :: General, task, P3)

Description

[Chris Peterson [:cpeterson]]

In bug 1436263, I added a cpp-virtual-final.yml linter to warn about virtual function declarations that included more than one virtual function specifier virtual, final, or override.

I think we should remove this linter now because:

• It's just a style check and doesn't diagnose a real bug. Including more than one virtual function specifier (virtual, final, or override) is harmless and unambiguous, just unnecessary extra code.
• It has caused some engineer frustration because this style check caused their changeset to be backed out of autoland. Backing out and fixing these style issues are not a good use of sheriffs' or engineers' time.
• It doesn't catch all virtual/final/override style issues because:
  • It can't analyze virtual function definitions that span multiple lines.
  • It doesn't check for virtual void Foo() override because there are over 6000 cases already, so our code will never follow this style check consistently.

Comment 1

[Masatoshi Kimura [:emk]]

(In reply to Chris Peterson [:cpeterson] from comment #0)

• It's just a style check and doesn't diagnose a real bug. Including more than one virtual function specifier (virtual, final, or override) is harmless and unambiguous, just unnecessary extra code.

As I wrote in bug 1436263, foo() final implies override while virtual foo() final does not. So it is not a harmless redundant code.

• It has caused some engineer frustration because this style check caused their changeset to be backed out of autoland. Backing out and fixing these style issues are not a good use of sheriffs' or engineers' time.

If we do not check

class A { virtual void foo(); };
class B: public A { virtual void foo() final; };

someone may change A::foo() without syncing B::foo(). The bug may not be noticed until the change is released and someone report the bug. Is it really better than immediate back outs?
We should have not changed final override to final if we stop checking virtual final.

• It doesn't catch all virtual/final/override style issues because:
  • It can't analyze virtual function definitions that span multiple lines.
  • It doesn't check for virtual void Foo() override because there are over 6000 cases already, so our code will never follow this style check consistently.

Why do we remove checks instead of improving them?

Comment 2

[Masatoshi Kimura [:emk]]

That said, virtual override, final override, and virtual final override are harmless. We can remove those checks, yes. Rather, we should force final override if we remove virtual final check.

Comment 3

[Chris Peterson [:cpeterson]]

Attachment: Bug 1756504 - Remove cpp-virtual-final linter, a minor style check that doesn't diagnose real bugs. r?#firefox-static-analysis-reviewers,#firefox-build-system-reviewers

In bug 1436263, I added a cpp-virtual-final.yml linter to warn about virtual function declarations that included more than one virtual function specifier virtual, final, or override.

I think we should remove this linter now because:

• It's just a style check and doesn't diagnose a real bug. Including more than one virtual function specifier (virtual, final, or override) is harmless and unambiguous, just unnecessary extra code.
• It has caused some engineer frustration because this style check caused their changeset to be backed out of autoland. Backing out and fixing these style issues are not a good use of sheriffs' or engineers' time.
• It doesn't catch all virtual/final/override style issues because:
  • It can't analyze virtual function definitions that span multiple lines.
  • It doesn't check for virtual void Foo() override because there are over 6000 cases already, so our code will never follow this style check consistently.

Comment 4

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/849eb13e577a
Remove cpp-virtual-final linter, a minor style check that doesn't diagnose real bugs. r=firefox-static-analysis-reviewers,sylvestre

Comment 5

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/849eb13e577a