search

bgpsecurity / rpstir

Relying Party Security Technology for Internet Routing
Other
9 stars 12 forks source link

miscellaneous cleanups #33

Closed rhansen closed 8 years ago

rhansen commented 8 years ago

see #27

dseomn commented 8 years ago

I've reviewed up to and including e64be6e, feel free to merge up to that point after addressing my comments. (I intend to finish reviewing the other commits later; I won't get to them tonight.)

dseomn commented 8 years ago

(nit) Why are the "use * type instead of int for " commits not next to the "use an enum for " commits in the graph?

dseomn commented 8 years ago

I've finished reviewing this, up to and including b954107. Overall it looks great!

rhansen commented 8 years ago

(nit) Why are the "use * type instead of int for " commits not next to the "use an enum for " commits in the graph?

All of the commits between those two batches are preparing for the "use * type instead of int for *" commits. Some of those commits depend on the new ERR_SCM_UNSPECIFIED error code, and I wanted to add that after converting the error codes to an enum so that I wouldn't need to renumber all of the #defines to make room for a new -1.

dseomn commented 8 years ago

On 2016-02-09 17:57, Richard Hansen wrote:

(nit) Why are the "use * type instead of int for " commits not next to the "use an enum for " commits in the graph?

All of the commits between those two batches are preparing for the "use * type instead of int for *" commits. Some of those commits depend on the new ERR_SCM_UNSPECIFIED error code, and I wanted to add that after converting the error codes to an enum so that I wouldn't need to renumber all of the #defines to make room for a new -1.

That's a good reason. Sorry, my eyes must have glazed over too much to notice that.

rhansen commented 8 years ago

I pushed a bunch of untested fixups that I believe address your comments except for your configure.ac comment. There are additional issues with configure.ac that I noticed, and it's easiest to deal with those in a rebase.

I'll test these untested fixups at some point. :smiley:

dseomn commented 8 years ago

I've reviewed all but the last of your fixups (8026c94^..6c8499a), and they look good.

rhansen commented 8 years ago

Rebased and ready for re-review. Aside from autosquashing, the only changes are in the commits affecting configure.ac. Only the commits touching configure.ac should be new to you.

dseomn commented 8 years ago

Merge it so!