[Einstein Toolkit] #936: Cactus binary tree implementation in src/util/BinaryTree.c is broken
#936: Cactus binary tree implementation in src/util/BinaryTree.c is broken ----------------------+----------------------------------------------------- Reporter: rhaas | Owner: Type: defect | Status: new Priority: optional | Milestone: Component: Cactus | Version: Keywords: | ----------------------+----------------------------------------------------- It contains an obvious bug of the form: {{{ int i = something;
if(i<0) {...} else if(i>0) {...} else if(i==0} {...} else {do something else} }}} which is clearly nonsensical (this is the second half of the patch). It also triggers segfaults since it blindly recurses into NULL pointers.
The second one concerns adding elements into the tree, which always compares to the prospective subtree's parent rather than the subtree itself.
No thorn seems to use these functions right now, nor are they documented.
#936: Cactus binary tree implementation in src/util/BinaryTree.c is broken -----------------------+---------------------------------------------------- Reporter: rhaas | Owner: Type: defect | Status: new Priority: optional | Milestone: Component: Cactus | Version: Resolution: | Keywords: -----------------------+----------------------------------------------------
Comment (by eschnett):
I suggest to remove this code, and to recommend to use std::map<,> instead.
#936: Cactus binary tree implementation in src/util/BinaryTree.c is broken -----------------------+---------------------------------------------------- Reporter: rhaas | Owner: Type: defect | Status: review Priority: optional | Milestone: Component: Cactus | Version: Resolution: | Keywords: -----------------------+---------------------------------------------------- Changes (by rhaas):
* status: new => review
#936: Cactus binary tree implementation in src/util/BinaryTree.c is broken -----------------------+---------------------------------------------------- Reporter: rhaas | Owner: Type: defect | Status: review Priority: optional | Milestone: Component: Cactus | Version: Resolution: | Keywords: -----------------------+----------------------------------------------------
Comment (by rhaas):
I second that. The binary tree implementation is odd even when making it work. It's interface is also awkward. I nice set of simple wrappers around std::map<,> sounds like a good idea (in particular if we can also provide Fortran wrappers since Fortran does not have any such thing right now).
#936: Cactus binary tree implementation in src/util/BinaryTree.c is broken -----------------------+---------------------------------------------------- Reporter: rhaas | Owner: Type: defect | Status: review Priority: optional | Milestone: Component: Cactus | Version: Resolution: | Keywords: -----------------------+----------------------------------------------------
Comment (by eschnett):
I would not wrap it in C code -- I would use it from C++. It is rather easy to convert C code to C++; what is mostly necessary is to add extern "C" around scheduled functions. I don't think it is necessary to provide Fortran wrapper; it will make more sense in most cases to implement the whole algorithm in C++, and only write e.g. compute kernels in Fortran.
#936: Cactus binary tree implementation in src/util/BinaryTree.c is broken -----------------------+---------------------------------------------------- Reporter: rhaas | Owner: Type: defect | Status: reviewed_ok Priority: optional | Milestone: Component: Cactus | Version: Resolution: | Keywords: -----------------------+---------------------------------------------------- Changes (by eschnett):
* status: review => reviewed_ok
Comment:
Removing the binary tree implementation and interface is approved.
#936: Cactus binary tree implementation in src/util/BinaryTree.c is broken -----------------------+---------------------------------------------------- Reporter: rhaas | Owner: Type: defect | Status: closed Priority: optional | Milestone: Component: Cactus | Version: Resolution: fixed | Keywords: -----------------------+---------------------------------------------------- Changes (by eschnett):
* status: reviewed_ok => closed * resolution: => fixed
Comment:
Done.
trac@lists.einsteintoolkit.org
-
Einstein Toolkit