Refactoring: namespace#7
Conversation
There is no good reason to put all these custom types into the global namespace, so this commit moves all queue implementation related code into a new namespace.
These types were re-defined by every implementation, but as they are part of the API of the tantrum queues, they should not be part of the individual implementations, but defined once and used by each implementation.
Instead of a hard-to-understand template, the missing function is added to the internal lock implementations that were missing it.
All remaining code that was in the default namespace is moved into namespace qd by this patch. For backwards compatibility, the default choices are exported into the default namespace, but users should switch from qdlock to qd::qdlock.
Adopt suggestions from pylint: - Use 4-space indentation instead of tabs. - Add docstrings for module and functions. - Capitalize constants. - Avoid using the same name for variables and function names. - Use snake_case for some variables. - Split long lines into multiple ones. In addition, use the `with ... as ...:` construct.
With the help of cpplint, the following code cleanups have been applied: - Use FILENAME_HPP_ to protect multiple inclusions of the header file. - Make sure that C headers appear before all C++ headers. - Include <utilty> header for move. - Delete unnecessary ; at the end of lines. - Mark with explicit constructors with a single argument.
| if(numa_available() != -1) { | ||
| numanodes = numa_num_configured_nodes(); | ||
| /* Initialize the NUMA map */ | ||
| for (int i = 0; i < num_cpus; ++i) { |
There was a problem hiding this comment.
Since we are re-writing this code, wouldn't be a better idea to use auto (or size_t) instead of int in these loops (and in similar ones below)?
Also, sysconf() returns a long, not an int. Perhaps you want to do something about this.
| if(found != nullptr) { | ||
| mynode->is_locked.store(mcs_node::taken, std::memory_order_release); | ||
| found->next = mynode; | ||
| for(int i = 0; i < 512; i++) { |
There was a problem hiding this comment.
Consider moving the 512 into a define or global constannt.
| if(found != nullptr) { | ||
| mynode->is_locked.store(mcs_node::taken, std::memory_order_release); | ||
| found->next = mynode; | ||
| for(int i = 0; i < 512; i++) { |
| } | ||
| }; | ||
| namespace qd { | ||
| namespace locks { |
There was a problem hiding this comment.
There is really no reason to indent nested namespaces. In fact, this practice is discouraged by many style checkers.
In C++17, it's possible to write nested namespaces using namespace A::B::C { ... } instead of namespace A { namespace B { namespace C { ... } } }.
My suggestion is to adopt the C++17 style of writing these.
| locked.store(true, std::memory_order_release); | ||
| } | ||
| }; | ||
| namespace qd { |
| } | ||
| void wake() {} | ||
| }; | ||
| namespace qd { |
| ticket_futex_lock() : ticket(0), serving(0) {} | ||
| ticket_futex_lock(ticket_futex_lock&) = delete; /* TODO? */ | ||
| namespace qd { | ||
| namespace locks { |
| static void wait_writers(base* t) { | ||
| while(reinterpret_cast<this_t>(t->__data)->writeBarrier.load() > 0) { | ||
| qd::pause(); | ||
| namespace qd { |
There was a problem hiding this comment.
My suggestion is to not indent after a namespace. It causes very long lines and unnecessary differences.
| counter.store(0, std::memory_order_relaxed); | ||
| closed.store(status::OPEN, std::memory_order_relaxed); | ||
| } | ||
| namespace qd { |
| template<long ENTRIES, int BUFFER_SIZE> | ||
| class entry_queue { | ||
| /** type for the size field for queue entries, loads must not be optimized away in flush */ | ||
| typedef std::atomic<long> sizetype; |
There was a problem hiding this comment.
Use some more well-defined type (e.g. int64) instead of long here?
(Similar comment about three lines above -- and also some lines below)
kostis
left a comment
kostis
left a comment
There was a problem hiding this comment.
I have left many suggestions, but they are just suggestions for improvement; not a prerequisite for merging this.
2 participants
This pull request adds namespaces to the codebase to avoid cluttering the default namespace, and avoid potential naming conflicts with user code.