Adding coderabbit artifacts#1052
Conversation
Signed-off-by: niranda perera <niranda.perera@gmail.com>
nirandaperera
added
improvement
pentschev
left a comment
pentschev
left a comment
There was a problem hiding this comment.
I think this is overall positive. I left some comments but I didn't read word-by-word, and I don't know if reviewers/humans are even expected to be as detailed on exact wording and examples being added. Are there any guidelines for authors/reviewers of changes like those proposed in this PR?
| - Use `RAPIDSMPF_EXPECTS` for precondition checks and `RAPIDSMPF_FAIL` | ||
| for unreachable / failure paths. The `RAPIDSMPF_EXPECTS` condition | ||
| must be a pure predicate with no side effects. |
There was a problem hiding this comment.
There are exceptions in cpp/src/bootstrap, see
rapidsmpf/cpp/src/bootstrap/file_backend.cpp
Lines 26 to 28 in 098dd1b
| - **C++**: Use the macros from | ||
| [`cpp/include/rapidsmpf/error.hpp`](cpp/include/rapidsmpf/error.hpp): | ||
| - `RAPIDSMPF_EXPECTS(cond, msg[, exception_type])` for precondition checks | ||
| (the condition must be a pure predicate — no side effects) | ||
| - `RAPIDSMPF_FAIL(msg[, exception_type])` for unreachable / failure paths | ||
| - `RAPIDSMPF_CUDA_TRY(cuda_call[, exception_type])` for every CUDA call | ||
| - `RAPIDSMPF_CUDA_TRY_ALLOC(cuda_call[, num_bytes])` for CUDA allocations | ||
| - `RAPIDSMPF_CUDA_TRY_FATAL(cuda_call)` / | ||
| `RAPIDSMPF_EXPECTS_FATAL(cond, msg)` in destructors / `noexcept` paths |
There was a problem hiding this comment.
Same here, exceptions for code that do not link to CUDA.
| 6. **Modern C++ / CCCL** — `cuda::std::` / libcudacxx in device code, | ||
| C++20 standard-library algorithms in host code. |
There was a problem hiding this comment.
Can/should we we be explicit stating "prefer C++20 over older standards when an alternative is available to simplify/improve code being proposed"?
wence-
left a comment
wence-
left a comment
There was a problem hiding this comment.
I'm not really very positive on this grab-bag of things that might easily go out of date as we refactor.
Can we just actually write documentation and get any of these models to look at it?
There was a problem hiding this comment.
Delete this file entirely. Instead capture specific patterns in skills if you must.
Signed-off-by: niranda perera <niranda.perera@gmail.com>
|
@wence- @pentschev I generalized the |
@wence- I am not really sure. Maybe we can set it as a prerequisite of the review. The coderabbit PRs in cudf and rmm were creating a static instruction set. So, I was mimicking those artifacts |
Any files you put in |
4 participants
Adding coderabbit artifacts based on the following PRs
Lets merge after 26.06 release