798634 - Minor mfbt/SHA1.* style cleanups, nits

Status: RESOLVED FIXED

(Core :: MFBT, defect)

Description

[Jeff Walden [:Waldo]]

Attachment: Patch

Mostly this is making it adhere more closely to mfbt/STYLE, but there are a couple things that go beyond that.

* proper #include ordering
* two-space indent for nested preprocessor directives
* |linkage type| on separate line from method name and arguments
* add a couple obvious assertions
* spaces around binary operators
* the "register" keyword is pointless
* C++-style casts
* empty lines between distinct blocks of code
* add docs to SHA1.h
* change the type of hashOut to be a reference to an array, not just a (decayed) pointer

And maybe one or two other little things, I only did a quick skim of the patch to write this comment.

There are two further things I'd like for this code, ideally, which I'm punting.

First, I wish this code used only fixed-width types, never natural types.  I think there are some implicit dependencies on things like |unsigned| being unsigned 32-bit integers or whatever; it'd be better if the dependencies were made explicit in the types uses.

Second, there are still some macros here which I'd prefer to see converted to inlines.

But for both cases, given I don't understand the algorithm at all, nor the interplay within this implementation, those changes seem more trouble than they're worth.  I guess this is what you get for copying screwy NSS code.  :-\

Comment 1

[Rafael Ávila de Espíndola (:espindola) (not reading bugmail)]

Why have you changed "static inline" to inline?

LGTM with that explained or changed back to "static inline".

Comment 2

[Jeff Walden [:Waldo]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a8a8a604511

Comment 3

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/2a8a8a604511