Added all DataMan plugins - #84
Conversation
|
@chuckatkins Please review and let me know if there is anything I need to change. Thanks. |
There was a problem hiding this comment.
Please remove the last merge commit. There were no conflicts so it's not necessary.
I git through ~ half the files but most of the patterns should be applied to the entire file set. The main highlights are: a) Use longer ifdef guards, b) Use { } on all blocks, even just 1 line, c) test pointers as bool instead of testing for null equality, d) use range for loops where applicable, child class constructors need to be virtual.
Also, the inline functions are really much to substantial. Please move the to their respective cpp files. in-header inline functions are really best suited for small functions called very frequently. Larger implementations like this are better suited to explicit implementation in the cpp file.
| * Author: Jason Wang | ||
| */ | ||
|
|
||
| #ifndef CACHEMAN_H_ |
There was a problem hiding this comment.
In order to avoid issues with conflicting include guards, we're starting to use ifdef guards based on the actual path, not just the filename. Please adjust all the include guards to use #ifdef DATAMAN_FILENAME_H_
| { | ||
| public: | ||
| CompressMan() = default; | ||
| ~CompressMan() = default; |
| this->add_next(method, man); | ||
| } | ||
| add_man_to_path("zfp", method); | ||
| if (p_jmsg["compress_method"] != nullptr) |
There was a problem hiding this comment.
Rather than test for nullptr, just treat the pointer as a bool, i.e. if(p_jmsg["compress_method"])
There was a problem hiding this comment.
This does not work with the JSON library. I have tried that.
There was a problem hiding this comment.
Bummer! That's okay. Skip that one then
There was a problem hiding this comment.
I have tried it again and I saw this:
libc++abi.dylib: terminating with uncaught exception of type std::domain_error: type must be boolean, but is null
Abort trap: 6
But I think I can replace them with if (msg["compress_method"].is_string())
There was a problem hiding this comment.
No, that's fine. Better to use nullptr
There was a problem hiding this comment.
I think .is_string() is the way that the library encourage people to use. It returns false if that object is nullptr, or that object is not a string. In either case it is safe to use.
| m_profiling["total_manager_time"].get<double>(); | ||
| if (p_jmsg["compressed_size"] != nullptr) | ||
| if (p_jmsg["compressed_size"].is_number()) | ||
| p_jmsg["putbytes"] = p_jmsg["compressed_size"].get<size_t>(); |
There was a problem hiding this comment.
Still use { and } for single line if blocks
|
|
||
| int DumpMan::init(json p_jmsg) | ||
| { | ||
| if (p_jmsg["dumping"] != nullptr) |
There was a problem hiding this comment.
Test for ptr as boolean, not by checking for nullptr equality
|
|
||
| // Pipes | ||
| mkdir(pipe_desc["pipe_prefix"].get<std::string>().c_str(), 0755); | ||
| for (int i = 0; i < pipe_desc["pipe_names"].size(); i++) |
There was a problem hiding this comment.
This is well suited to convert to a range-for
|
|
||
| // push new request | ||
| jqueue.push(jmsg); | ||
| bqueue.push(NULL); |
| { | ||
| // allocate buffer | ||
| size_t putbytes = jqueue.front()["putbytes"].get<size_t>(); | ||
| if (bqueue.front() == NULL) |
|
|
||
| // determine the pipe for the head request | ||
| json msg = jqueue.front(); | ||
| if (msg == nullptr) |
There was a problem hiding this comment.
Use ptr as bool. Also add {}
| putbytes = msg["putbytes"].get<int>(); | ||
| while (s < putbytes) | ||
| { | ||
| int ret = read(pipes[pipeindex], ((char *)bqueue.front()) + s, |
c5c326c to
90792f7
Compare
|
@chuckatkins I have changed everything as requested. Please review. |
|
I found a few more places where I should have added brackets. |
|
@chuckatkins I fixed a few other problems which might cause errors at compile time. I wasn't able to check whether it compiles on the machine I was using in the afternoon because of the pthread thing not merged in. Sorry about that. This time it should compile and work. |
|
@JasonRuonanWang I've rebased on master to eliminate all of the merges into this topic branch. Please look at the dataman-rebased branch on my fork. If it looks good to you then I'll force-push it onto yours to update the pr, and then ill merge it. |
|
@chuckatkins Yes, it looks all good to me. Please go ahead. |
Code extracted from:
https://github.com/GTkorvo/dill.git
at commit ce161228716eb5b620676751a4f797e8ad06c536 (master).
Upstream Shortlog
-----------------
Greg Eisenhauer (1):
ce161228 Add global register pre-assignment for virtual mode loops (ornladios#84)
Code extracted from:
https://github.com/GTkorvo/dill.git
at commit ce161228716eb5b620676751a4f797e8ad06c536 (master).
Upstream Shortlog
-----------------
Greg Eisenhauer (1):
ce161228 Add global register pre-assignment for virtual mode loops (ornladios#84)
Code extracted from:
https://github.com/GTkorvo/dill.git
at commit 9d32c1df536547e8b782a899a3d16697f978f55c (master).
Upstream Shortlog
-----------------
Greg Eisenhauer (4):
ce161228 Add global register pre-assignment for virtual mode loops (ornladios#84)
1929b841 ARM64 built-in disassembler for dill_dump (ornladios#85)
9afe8637 Restore JIT execute mode in finalize_package and free_stream (ornladios#86)
9d32c1df Fix ARM64 Linux support (ornladios#87)
2 participants
No description provided.