4 ms·

The code review here is excellent and supportive. I'm very impressed.

by moomin 10y ago

ekidd 10y ago
I've submitted a couple of small patches and the Rust core team has always been very helpful and pleasant.
- kzrdude 10y ago
  In this case of the sort improvement, nobody in the literal Rust core team was involved in reviewing the PR.
- thegeomaster 10y ago
  I once submitted a patch to Cargo. Despite being uninformed and violating code style rules which I should have read and known about, they patiently guided me, I resolved the issues, and it ended up being merged. They're great folks.
wyldfire 10y ago
IMO the LLVM community could learn a few things from the Rust team. I know that the activity level is not the same but it's really encouraging to new folks that reviews get engagement and disposition: "this is good, but fix that" instead of wilting without any clear indication of whether it could be merged. Also, the fact that the CI triggers and lets you know whether your change causes any collateral damage before you merge takes away a lot of the fear factor of contributing. This is also in stark contrast to LLVM's "merge it if it LGTM, revert it soon after when we find out what it broke."
- mastax 10y ago
  As an example of this, Zoxc has been trying to get stack probe support into LLVM for two years to no avail [0]. This is important to make sure that stack overflow or buffer overrun actually trigger the guard page and cause termination. I admit it's not really fair to LLVM to pick on one patch like this but it's frustrating to me even as an outsider. [0]: https://reviews.llvm.org/p/Zoxc/ https://reviews.llvm.org/p/Zoxc/
  - wyldfire 10y ago
    Is that just a stack canary? Hasn't gcc had this for years?
    - pcwalton 10y ago
      No, stack canaries are different. Stack canaries defend against some classes of buffer overflows. Stack probes defend against some classes of stack overflows.
  - whitequark_ 10y ago
    Zoxc has repeatedly refused reasonable requests to alter the patch to fit the standards of upstream. Because of this (and because I am rather tired of not having stack probes, with my embedded system not having functional guard pages), I'll make a hostile fork of their patch soon and get it through the pipeline. Stay tuned.
    - mastax 10y ago
      Ah, interesting. I hadn't gleamed that from the threads. It's great to hear that there's a chance that this will be supported though!
    - dbaupp 10y ago
      Late comment, but aren't stack probes entirely predicated on having working guard pages? I.e. they only exist to guarantee that the guard page is accessed before anything else, when a frame is larger than the page.
pow_pp_-1_v 10y ago
The PR author's initial write-up and responses to comments were excellent too.
mathw 10y ago
That struck me, too. I've found the Rust community generally very helpful, but this is really superb feedback and discussion.

The code review here is excellent and supportive. I'm very impressed.

by moomin 10y ago

The code review here is excellent and supportive. I'm very impressed.

ekidd 10y ago

I've submitted a couple of small patches and the Rust core team has always been very helpful and pleasant.

kzrdude 10y ago
In this case of the sort improvement, nobody in the literal Rust core team was involved in reviewing the PR.
thegeomaster 10y ago
I once submitted a patch to Cargo. Despite being uninformed and violating code style rules which I should have read and known about, they patiently guided me, I resolved the issues, and it ended up being merged. They're great folks.

wyldfire 10y ago

IMO the LLVM community could learn a few things from the Rust team. I know that the activity level is not the same but it's really encouraging to new folks that reviews get engagement and disposition: "this is good, but fix that" instead of wilting without any clear indication of whether it could be merged. Also, the fact that the CI triggers and lets you know whether your change causes any collateral damage before you merge takes away a lot of the fear factor of contributing. This is also in stark contrast to LLVM's "merge it if it LGTM, revert it soon after when we find out what it broke."

mastax 10y ago
As an example of this, Zoxc has been trying to get stack probe support into LLVM for two years to no avail [0]. This is important to make sure that stack overflow or buffer overrun actually trigger the guard page and cause termination. I admit it's not really fair to LLVM to pick on one patch like this but it's frustrating to me even as an outsider. [0]: https://reviews.llvm.org/p/Zoxc/ https://reviews.llvm.org/p/Zoxc/
- wyldfire 10y ago
  Is that just a stack canary? Hasn't gcc had this for years?
  - pcwalton 10y ago
    No, stack canaries are different. Stack canaries defend against some classes of buffer overflows. Stack probes defend against some classes of stack overflows.
- whitequark_ 10y ago
  Zoxc has repeatedly refused reasonable requests to alter the patch to fit the standards of upstream. Because of this (and because I am rather tired of not having stack probes, with my embedded system not having functional guard pages), I'll make a hostile fork of their patch soon and get it through the pipeline. Stay tuned.
  - mastax 10y ago
    Ah, interesting. I hadn't gleamed that from the threads. It's great to hear that there's a chance that this will be supported though!
  - dbaupp 10y ago
    Late comment, but aren't stack probes entirely predicated on having working guard pages? I.e. they only exist to guarantee that the guard page is accessed before anything else, when a frame is larger than the page.

pow_pp_-1_v 10y ago

The PR author's initial write-up and responses to comments were excellent too.

mathw 10y ago

That struck me, too. I've found the Rust community generally very helpful, but this is really superb feedback and discussion.