FreeBSD port fixes for RenderDoc #3733
Conversation
xinitrcn1
changed the title
baldurk
left a comment
baldurk
left a comment
There was a problem hiding this comment.
Please look through the contributing documentation in particular how to prepare a branch and commits to comply with the clang-formatting and history expectations for a PR.
renderdoc/os/os_specific.h
Outdated
| ~Semaphore(); | ||
|
|
||
| private: | ||
| #if !defined(__APPLE__) && !defined(_WIN32) && !defined(__linux__) |
There was a problem hiding this comment.
Please do not add new declarations and preprocessor conditionals like this into os_specific.h
renderdoc/os/os_specific.h
Outdated
| #if !defined(__APPLE__) && !defined(_WIN32) && !defined(__linux__) | ||
| std::mutex m_semaphore_mutex{}; | ||
| std::condition_variable m_condition_variable{}; | ||
| uint32_t m_count{}; |
There was a problem hiding this comment.
Please use normal initialisers like uint32_t m_count = 0; instead of this kind of syntax.
There was a problem hiding this comment.
for non trivial initializers should i = {};
| return 1; | ||
| } | ||
|
|
||
| return static_cast<uint32_t>(core_count); |
There was a problem hiding this comment.
Please use normal casts not C++ casts.
| } | ||
|
|
||
| Threading::Semaphore::Semaphore() = default; | ||
| Threading::Semaphore::~Semaphore() = default; |
There was a problem hiding this comment.
Please do not use this syntax, if declaring a default constructor do so in the class declaration.
| return; | ||
| } | ||
|
|
||
| std::unique_lock<std::mutex> lock(m_semaphore_mutex); |
There was a problem hiding this comment.
Please do not add new STL usage. The point of this class is to implement the semaphore primitive using OS details such as pthreads, not add an STL dependency.
There was a problem hiding this comment.
pthreads is an extra dependency on FBSD, whereas STL comes by default.
FBSD also doesn't have "proper" semaphores with pthreads (14x had an issue with high contention and whatnot).
I'll use pthreads anyways through.
|
This should do now, no STL, just sem_* (available since fbsd 5 so it's fine), only difference is |
baldurk
left a comment
baldurk
left a comment
There was a problem hiding this comment.
The code looks fine now but the branch is still not ready for merging, please ensure it complies with the guidelines in CONTRIBUTING.
|
Closing this due to lack of activity from the PR author and no response to feedback. If you are the author and you'd like to revisit this change please open a new PR and address the feedback above. |
I did answer to your feedback and that was 9f525e3 - I'm not sure what guidelines I'm missing now? Please revise the new filers as well |
|
As in the comment above the branch is not ready for merging as mentioned in my original code review. Please read the CONTRIBUTING guidelines and ensure you have prepared the branch properly and open a new PR once it's ready. |
2 participants
Works flawlessly except that FBSD has some issues with older NVIDIA cards because NVIDIA drivers suck.
But lavapipe and all the fun family of vulkan stuff is working as expected :)