[llvm-commits] [llvm] r44851 - /llvm/trunk/include/llvm/ADT/Trie.h

[Wojciech Matyjewicz]

Anton Korobeynikov wrote:
> +template<class Payload>
> +class Trie {
> +  class Edge;
> +  class Node;
> +
> +  class Edge {
> +    std::string Label;
> +    Node *Parent, *Child;

The Parent field seems to be unused internally by the Trie
implementation. Assuming the Edge interface is invisible for Trie
clients, may this field be removed?

> +  std::vector<Node*> Nodes;

I guess this vector is purely for destruction purposes... Have you
considered storing a root node only and performing destruction by
postorder trie traversal?

> +  inline Node* getRoot() const { return Nodes[0]; }

I'm not familiar what Trie structure will be used for, but shouldn't
this method be private? Currently, it exposes a way to call most of the
Node's (and indirectly Edge's) methods, which I suppose, should be
inaccessible to clients.

> +  bool addString(const std::string& s, const Payload& data) {
> +    Node* cNode = getRoot();
> +    Edge* nEdge = NULL;
> +    std::string s1(s);
> +
> +    while (nEdge == NULL) {
> +      if (cNode->Edges.count(s1[0])) {
> +        Edge& cEdge = cNode->Edges[s1[0]];
> +        typename Edge::QueryResult r = cEdge.query(s1);
> +
> +        switch (r) {
> +        case Edge::Same:

Do we really want to hit the assert here, and not only return false?

> +        case Edge::StringIsPrefix:

Shouldn't the edge be splitted in this case and the payload be attached
to the new node?

-Wojtek