feat: Add acyclic and tred support #211
Conversation
src-cpp/graphviz/main.cpp
Outdated
| { | ||
| if (agtail(e) != aghead(e)) | ||
| rv++; | ||
| FILE *outFile = fopen("tmp.dot", "w"); |
There was a problem hiding this comment.
Do you not want to (1) use a getenv("TMPDIR") || "/tmp" prefix, (2) use mkstemp or something to ensure parallel executions won’t interfere with each other, and (2) check for failure here?
Alternative to using a temporary file, if this only runs on !Windows, you could do this entirely in memory with open_memstream.
There was a problem hiding this comment.
FYI neither of these are really a concern in WebAssembly as the files end up in memory anyway (and parallel execution will only happen with separate instances).
I will tidy up into a convenience class anyway as this is code is getting reproduced a few times now...
src-ts/graphviz.ts
Outdated
| @@ -155,24 +156,90 @@ export class Graphviz { | |||
| return retVal; | |||
| } | |||
|
|
|||
| /** | |||
| * acyclic is a filter that takes a directed graph as input and outputs a copy of the graph with sufficient edges reversed to make the graph acyclic. The reversed edge inherits all of the attributes of the original edge. The optional file argument specifies where the the input graph is stored; by default. | |||
src-ts/graphviz.ts
Outdated
| * @param dotSource Required - graph definition in [DOT](https://graphviz.gitlab.io/doc/info/lang.html) language | ||
| * @param doWrite Enable output is produced, though the return value will indicate whether the graph is acyclic or not. | ||
| * @param verbose Print information about whether the file is acyclic, has a cycle or is undirected. | ||
| * @returns `{ acyclic: boolean, num_rev: number, outFile: string }` Boolean will be true if a cycle was found, as well as. |
There was a problem hiding this comment.
nit: double space. Though it actually looks like there are more words missing here.
GordonSmith
force-pushed
the
GV_10
branch
3 times, most recently
from
5835f3c
to
4683ebd
Compare
Refactor graphviz to common up buffer usage Fixed typos in javadoc Added some simple tests for async and tred Fixed error reporting for unflatten Fixes hpcc-systems#210 Signed-off-by: Gordon Smith <GordonJSmith@gmail.com>
| public: | ||
| TempFileBuffer(); | ||
| ~TempFileBuffer(); |
There was a problem hiding this comment.
You probably want to = delete the copy/move constructor/assignment for this class, right?
|
|
||
| #include <string> | ||
|
|
||
| class StringBuffer |
There was a problem hiding this comment.
Not sure I understand the intent of this class. Why not just use a std::string?
There was a problem hiding this comment.
Might be a naming issue (StringAdapter might be better), but this allows the IDL generated "glue" code to return a const char * directly, without having to declare an explicit const char * member.
| class TempFileBuffer | ||
| { | ||
| private: | ||
| FILE *filePointer = nullptr; |
| @@ -1,10 +1,9 @@ | |||
| #include <string> | |||
| #include "util.hpp" | |||
|
|
|||
| class Graphviz | |||
| { | |||
| private: | |||
There was a problem hiding this comment.
nit: I think you could also remove this line.
| if (filePointer != nullptr) | ||
| { | ||
| std::rewind(filePointer); | ||
| char buffer[256]; |
There was a problem hiding this comment.
nit: maybe you want to use BUFSIZ for the size here.
There was a problem hiding this comment.
Should be able to remove the buff and loop altogether...
Fixes #210