Add "override" and "final" to classes and methods in mozilla::pkix

RESOLVED FIXED in mozilla38

Status

()

--
enhancement
RESOLVED FIXED
4 years ago
4 years ago

People

(Reporter: briansmith, Assigned: briansmith)

Tracking

Trunk
mozilla38
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(4 attachments)

* Every method *implementation* (methods that aren't abstract) in mozilla::pkix should be annotated with "final". We never override a non-abstract method in mozilla::pkix.

* Every method implementation in mozilla::pkix that implements an abstract method defined in the base class should also be annotated with "override".

* We need to have logic to handle the fact that GCC, before GCC 4.7, does not support "final" or "override", something like this (in enumclass.h, which whould need to be renamed to something like old-compiler-workarounds.h):

 #if defined(__GNUC__) && (__GNUC__ * 100 + __GNUC_MINOR__ < 407)
 // GCC before version 4.7 may crash when compiling code that static_casts a
 // value of scoped typed enum type. See
 // https://gcc.gnu.org/bugzilla/show_bug.cgi?id=48106.
 #define MOZILLA_PKIX_ENUM_CLASS enum
+// GCC before version 4.7 does not support "final" or "override"
+#define final
+#define override
 #else
Created attachment 8548052 [details] [diff] [review]
final-override.patch
Assignee: nobody → brian
Status: NEW → ASSIGNED
Attachment #8548052 - Flags: review?(dkeeler)
Comment on attachment 8548052 [details] [diff] [review]
final-override.patch

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

Oh, I see - the patch in bug 1115910 depends on this one.
I'm assuming you're working on an additional patch to actually use the keywords where appropriate? (i.e. this patch seems necessary but not sufficient to complete the goal of this bug)

::: security/certverifier/ExtendedValidation.cpp
@@ +9,5 @@
>  #include "cert.h"
>  #include "certdb.h"
>  #include "base64.h"
>  #include "hasht.h"
> +#include "pkix/stdkeywords.h"

It would be nice to keep these sorted (i.e. this would go after pkix/pkixtypes.h here and in PublicKeyPinningService.cpp)
Attachment #8548052 - Flags: review?(dkeeler) → review+
Created attachment 8548337 [details] [diff] [review]
Part 2: Add "final" and "override" to mozilla::pkix classes

I will re-sort the #includes before checkin.
Attachment #8548337 - Flags: review?(dkeeler)
Comment on attachment 8548337 [details] [diff] [review]
Part 2: Add "final" and "override" to mozilla::pkix classes

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

LGTM. Do we care about things like struct CertID?
Attachment #8548337 - Flags: review?(dkeeler) → review+
Created attachment 8548551 [details] [diff] [review]
Part 3: Format enums, classes, and structs more consistently
Attachment #8548551 - Flags: review?(dkeeler)
Created attachment 8548553 [details] [diff] [review]
Part 2(b): Add "final" to more stuff

Thanks for noticing that I overlooked these.
Attachment #8548553 - Flags: review?(dkeeler)
Comment on attachment 8548551 [details] [diff] [review]
Part 3: Format enums, classes, and structs more consistently

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

r=me

::: security/pkix/include/pkix/pkixtypes.h
@@ +82,5 @@
>  
>    void operator=(const SignedDataWithSignature&) = delete;
>  };
>  
>  enum class EndEntityOrCA { MustBeEndEntity = 0, MustBeCA = 1 };

Just so we're clear - I'm inferring that the style for enum class definitions is to put them all on one line if it fits (I think there are some instances of this in pkixnames.cpp too).
Attachment #8548551 - Flags: review?(dkeeler) → review+
Comment on attachment 8548553 [details] [diff] [review]
Part 2(b): Add "final" to more stuff

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

Great - r=me.
Attachment #8548553 - Flags: review?(dkeeler) → review+
https://hg.mozilla.org/integration/mozilla-inbound/rev/75174df7140a
https://hg.mozilla.org/integration/mozilla-inbound/rev/5df5279f3ad8
https://hg.mozilla.org/integration/mozilla-inbound/rev/edacb151e7ac

(In reply to David Keeler [:keeler] (use needinfo?) from comment #7)
> Just so we're clear - I'm inferring that the style for enum class
> definitions is to put them all on one line if it fits (I think there are
> some instances of this in pkixnames.cpp too).

Yes, that is the pattern I've been following.

Thanks for the quick reviews!
Target Milestone: --- → mozilla38
Depends on: 1146057
You need to log in before you can comment on or make changes to this bug.