Skip to content

Conversation

@gaspardruan
Copy link

What type of PR is this?

refactor: A code change that neither fixes a bug nor adds a feature

Check the PR title.

  • This PR title match the format: <type>(optional scope): <description>
  • The description of this PR title is user-oriented and clear enough for others to understand.
  • Attach the PR updating the user documentation if the current PR requires user awareness at the usage level. User docs repo

(Optional) More detailed description for this PR(en: English/zh: Chinese).

Changes:

  1. "/" -> slash, ':' -> paramLabel, '*' -> anyLabel
if lcpLen == 0 {
	// At root node
	currentNode.label = search[0]
	currentNode.prefix = search
	if h != nil {
		currentNode.kind = t
		currentNode.handlers = h
		currentNode.ppath = ppath
		currentNode.pnames = pnames
	}

--	currentNode.isLeaf = currentNode.children == nil && currentNode.paramChild == nil && currentNode.anyChild == nil
++	currentNode.isLeaf = true
}

Only one possible control flow into the block, that is the first time into the for loop. And it's impossible from the continue , because c is not nil and the search and currentNode.prefix must have the same start char, then lcpLen > 0. And search := path must start with "/", so when lcpLen == 0, currentNode.prefix is empty, which means it's a root node. So it must also be a leaf node.

search = search[lcpLen:]
c := currentNode.findChildWithLabel(search[0])
if c != nil {
  // Go deeper
  currentNode = c
  continue
}
  1. currentNode.children = append(currentNode.children, n) already indicated that currentNode.isLeaf = false, so it's proper to delete it because it had been reseted: currentNode.isLeaf = false
    // Reset parent node
    currentNode.kind = skind
    currentNode.label = currentNode.prefix[0]
    currentNode.prefix = currentNode.prefix[:lcpLen]
    currentNode.children = nil
    currentNode.handlers = nil
    currentNode.ppath = nilString
    currentNode.pnames = nil
    currentNode.paramChild = nil
    currentNode.anyChild = nil
    currentNode.isLeaf = false
    
    // Only Static children could reach here
    currentNode.children = append(currentNode.children, n)

    if lcpLen == searchLen {
        // At parent node
        currentNode.kind = t
        currentNode.handlers = h
        currentNode.ppath = ppath
        currentNode.pnames = pnames
    } else {
        // Create child node
        n = newNode(t, search[lcpLen:], currentNode, nil, h, ppath, pnames, nil, nil)
        // Only Static children could reach here
        currentNode.children = append(currentNode.children, n)
    }
-- currentNode.isLeaf = currentNode.children == nil && currentNode.paramChild == nil && currentNode.anyChild == nil
  1. Obviously.
switch t {
	case skind:
		currentNode.children = append(currentNode.children, n)
	case pkind:
		currentNode.paramChild = n
	case akind:
		currentNode.anyChild = n
}
--	currentNode.isLeaf = currentNode.children == nil && currentNode.paramChild == nil && currentNode.anyChild == nil
++	currentNode.isLeaf = false

@gaspardruan gaspardruan requested review from a team as code owners September 10, 2025 14:13
@xiaost
Copy link
Contributor

xiaost commented Sep 10, 2025

Thanks for feedback.

I believe currently no one can help review your code,
and the previous code was clearer than yours because your code requires extensive explanation to be understood.

If we have no improvement for such minor change, we would rather remain unchanged.
If there is any issue, you can open one in Issues page.

Thanks again, good job anyway 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Development

Successfully merging this pull request may close these issues.

2 participants