1125673 - pkixocsp_VerifyEncodedOCSPResponse.cpp:50:10: warning: 'FindIssuer' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]

Status: RESOLVED FIXED

(Core :: Security: PSM, defect)

Description

[Daniel Holbert [:dholbert]]

Clang 3.6 build warning:
{
security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp:50:10: warning: 'FindIssuer' overrides a member function but is not marked 'override' [-Winconsistent-missing-override]

   Result FindIssuer(Input, IssuerChecker&, Time)
          ^
}

Patch coming up to mark this function as "final override", like all the methods alongside it.

Comment 1

[Daniel Holbert [:dholbert]]

Attachment: fix v1

Comment 2

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Comment on attachment 8554328 [details] [diff] [review]
fix v1

Review of attachment 8554328 [details] [diff] [review]:
-----------------------------------------------------------------

::: security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp
@@ +46,5 @@
>      trustLevel = TrustLevel::InheritsTrust;
>      return Success;
>    }
>  
> +  Result FindIssuer(Input, IssuerChecker&, Time) final override

Please follow the Google C++ Style Guide: "For clarity, use exactly one of override, final, or virtual when declaring an override," in this case just "final".

(It seems NSS has adopted the Google C++ style guide instead of the Mozilla style guide, and mozilla::pkix will eventually end up in NSS, so I try to follow the Google C++ style guide except where it's dumb to do so.)

Comment 3

[Daniel Holbert [:dholbert]]

Hmm, ok -- that makes sense. (I haven't used "final" much, & hadn't realized that it was redundant when used with "override".)

FWIW, I was actually aiming to follow local style here -- we've got "final override" on all of the adjacent functions. (You added those annotations in http://hg.mozilla.org/mozilla-central/diff/5df5279f3ad8/security/pkix/test/gtest/pkixocsp_VerifyEncodedOCSPResponse.cpp )  So if you really want this file to match that coding-style guideline, then it seems the existing "final overrides" need fixing as well. :)

Comment 4

[(no longer active)]

final and override mean very different things.  It makes a lot of sense for a coding style to require not using virtual with either of the override (because it cannot be used on anything not overriding a virtual function) and final (because it makes no sense to have a virtual function that is not overriding anything and is final, and otherwise it is virtual by default.)

However it makes perfect sense to say something is an override, and it cannot be further overridden (final).  So we should not adhere to that part of the Google C++ style.

Comment 5

[Daniel Holbert [:dholbert]]

For reference, the Google C++ Style Guide point in question is:
https://google-styleguide.googlecode.com/svn/trunk/cppguide.html?showone=The__define_Guard#Inheritance

Before writing comment 3, I read up on final here: http://en.cppreference.com/w/cpp/language/final , and I initially read it as implying that "final" and "override" were redundant - but on re-reading, I see that they indeed are *not* redundant.

In particular, this compiles just fine:
  class Foo {
    virtual void DoStuff() final;
  };
...but this does not:
  class Foo {
    virtual void DoStuff() final override;
  };

So, "override" does buy us some sanity-checking here, and we should absolutely keep it.  (Moreover, it seems like "final" on its own will not address the build warning in comment 0.)

Brian, are you ok with the patch as-is, given that (& given that it matches your local style :))?

Comment 6

[Daniel Holbert [:dholbert]]

(In reply to Daniel Holbert [:dholbert] from comment #5)
> In particular, this compiles just fine:

...though ah, I suppose a better example (for the purposes of this coding-style rule) is:
 class Foo {
   void DoStuff() final;
 };
...which does not compile (and will not compile, unless it happens to override and inherit some virtualness from the parent class).

I suppose the coding-style rule makes some sense, in light of that. So, I'm OK going either way on this.

> (Moreover, it seems like "final" on its own will not
> address the build warning in comment 0.)

Sorry, I just tested this, and it does seem that "final" on its own does fix the build warning.  So I can indeed apply the review comment & still fix this bug.

Brian, any thoughts? I'm good going either way on this. (Though until we've made a 100% decision to fix this whole file to remove 'final override' annotations, I marginally lean towards that, for internal consistency.)

Comment 7

[Brian Smith (:briansmith, :bsmith, use NEEDINFO?)]

Yes, I think it is better the way you originally wrote the patch. According to the Google style guide, nobody should ever write "virtual void DoStuff() final" because it breaks the rule, but if somebody *does* write that, then getting a compiler error is better than silently accepting the weird code. Sorry for the bothering.

Comment 8

[Daniel Holbert [:dholbert]]

Cool, thanks for the review & thanks bringing up that coding-style point anyway; good food for thought about how these annotations interact.

Comment 9

[Daniel Holbert [:dholbert]]

Landed: https://hg.mozilla.org/integration/mozilla-inbound/rev/d91343f88f11

Comment 10

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/d91343f88f11