Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

UTF8-Patch V2 2015.09.21 #31

Closed
wants to merge 2 commits into from
Closed

UTF8-Patch V2 2015.09.21 #31

wants to merge 2 commits into from

Conversation

BestianCode
Copy link
Contributor

Unicode patch for ldap-search. I use "unicode/utf8" library for it.

  1. I copied "filter" to "xfilter"
  2. First "for" cut symbols from string to "newPos" position;
  3. Cut "xfilter" for one symbol (not byte) per step and append to "condition"

@liggitt
Copy link
Contributor

liggitt commented Sep 21, 2015

Thanks for the PR. I think a better approach would be to switch to using http://golang.org/pkg/unicode/utf8/#DecodeRuneInString to get the "next" character in the filter, rather than using substring-based extraction

@liggitt
Copy link
Contributor

liggitt commented Sep 21, 2015

Something like this, for example (consider this pseudo-code... just a sketch of an approach):

nextRune, width := utf8.DecodeRuneInString(filter[pos:])
newPos = newPos + width
switch nextRune {
case utf8.RuneError:
  return nil, 0, utf8.RuneError
case rune('('):
  packet, newPos, err = compileFilter(filter, newPos)
...
default:
  for {
    nextRune, width := utf8.DecodeRuneInString(filter[pos:])
    switch nextRune {
    case utf8.RuneError:
      return nil, 0, utf8.RuneError
    case rune(')'):
      break
    default:
      // use nextRune instead of filter[newPos+1]
    }
  }
}

@BestianCode
Copy link
Contributor Author

Hmm... I will correct :)

@BestianCode BestianCode changed the title UTF-8 Feature UTF8-Patch V2 2015.09.21 Sep 22, 2015
@liggitt
Copy link
Contributor

liggitt commented Sep 22, 2015

No problem… in the future, you can push updates to your existing branch and the pull request will update itself, no need to close and reopen another one

@BestianCode
Copy link
Contributor Author

Ok, thank you :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants