Skip to content

Conversation

wdullaer
Copy link
Contributor

@wdullaer wdullaer commented Sep 6, 2024

The standard Identifier function used by the generator results in very unidiomatic go identifier names when used with my ebpf object. With this change, the Identifier function becomes configurable, allowing me to pass in something that produces better outputs in my context. If no function is passed in, we keep using the default, this makes the change entirely backwards compatible.
Furthermore, a test has been added to ensure the configuration is correctly passed through.

@wdullaer wdullaer requested a review from mejedi as a code owner September 6, 2024 16:29
@wdullaer wdullaer force-pushed the custom-identifier-function branch from a519023 to eb8aaf3 Compare September 6, 2024 16:29
Copy link
Collaborator

@ti-mo ti-mo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know where we stand regarding using bpf2go as a library, or if this fits the vision for the project. Deferring to @mejedi and @lmb for that.

@wdullaer Could you provide some examples of unusual output of internal.Identifier? Maybe we could improve the default instead, which could benefit all users.

@wdullaer wdullaer force-pushed the custom-identifier-function branch from eb8aaf3 to 48f2ffb Compare September 11, 2024 10:03
@wdullaer
Copy link
Contributor Author

We use ebpf programs written in rust, which means the identifiers follow rust convention. Specifically map names are all caps and field names have underscores.

We're using this function:

// Identifier turns a C style type or field name into an exportable Go equivalent.
func Identifier(s string) string {
	s = strings.TrimSpace(s)
	if s == "" {
		return s
	}

	capNext := true
	prevIsCap := false
	return strings.Map(func(v rune) rune {
		vIsCap := unicode.IsUpper(v)
		if capNext {
			if unicode.IsLower(v) {
				v = unicode.ToUpper(v)
			}
			vIsCap = true
		} else if prevIsCap && unicode.IsUpper(v) {
			v = unicode.ToLower(v)
		}
		prevIsCap = vIsCap

		if unicode.IsLetter(v) {
			capNext = false
			return v
		} else if unicode.IsNumber(v) {
			capNext = true
			return v
		} else {
			capNext = v == '_' || unicode.IsSpace(v) || v == '-' || v == '.'
			return -1
		}
	}, s)
}

I would be careful with changing the default here, since this might cause churn in clients.
I understand this is very much an aesthetic thing and the original bikeshed :-)

The standard Identifier function used by the generator results in very
unidiomatic go identifier names when used with my ebpf object.
With this change, the Identifier function becomes configurable, allowing
me to pass in something that produces better outputs in my context.
If no function is passed in, we keep using the default, this makes the
change entirely backwards compatible.
Furthermore, a test has been added to ensure the configuration is
correctly passed through.

Signed-off-by: Wouter Dullaert <wouter.dullaert@exoscale.ch>
@wdullaer wdullaer force-pushed the custom-identifier-function branch from 48f2ffb to 74774ce Compare September 13, 2024 11:15
Copy link
Contributor

@mejedi mejedi left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@ti-mo ti-mo merged commit 3a00452 into cilium:main Sep 24, 2024
17 checks passed
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.

3 participants