Scott Schurr

Results 107 comments of Scott Schurr

I have not looked closely at this pull request. That said, I'm not convinced that the proposed change brings enough value to justify... 1. The risk associated with the change,...

@ckeshava, thanks for that documentation. That helps. And I too would appreciate hearing @pwang200's opinion regarding this change. Regarding the three points you bring up: 1. Correctness: you are citing...

Thanks for the review @cjcobb23! I've squashed and rebased the commit and marked it as passed.

@intelliot, no, I can't. I have another higher priority code review I'm working on. I can't provide an ETA for when I'll get to this one.

It seems like you're running into some problems interfacing with `boost::beast`, since it is inclined to use `boost::string_view` instead of `std:::string_view`. Boost must support many users who are stuck on...

@ckeshava, while working on this review I noticed that there are still some `std::string_view const&` parameters. I _think_ that the article from @thejohnfreeman convinced you that passing `std::string_view` by value...

Huh. Just FYI, I ran across a couple of MSVC intrinsics today: `_udiv128` and `_umul128`. They are both available in MSVC 2017 which is probably the oldest compiler MS compiler...

@intelliot, In addition to code reviews, there's an additional concern regarding this pull request. I think this pull request starts us down a good road in terms of compile-time coding...

@ximinez, working with a feature branch would be possible. However I have concerns: - Converting all of the ledger types will touch a _lot_ of stuff. Review of the final...

From @intelliot: > What's your opinion? Perhaps, after taking a step back and considering all the effort required (including reviews), we've learned that this refactor is not worthwhile at this...