* 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

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)

I will re-sort the #includes before checkin.

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?

Thanks for noticing that I overlooked these.

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).

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.

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!

https://hg.mozilla.org/mozilla-central/rev/75174df7140a https://hg.mozilla.org/mozilla-central/rev/5df5279f3ad8 https://hg.mozilla.org/mozilla-central/rev/edacb151e7ac