Skip to content

enabled fieldalign in govet settings #664

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

Closed
wants to merge 2 commits into from

Conversation

skoef
Copy link
Contributor

@skoef skoef commented Jan 17, 2022

As mentioned in #663 I enabled the fieldalignment linter in govet and aligned all mis-aligned structs. This was done mostly by using the fieldalignment tool and running it:

% go install golang.org/x/tools/go/analysis/passes/fieldalignment/cmd/fieldalignment@latest
% $GOPATH/bin/fieldalignment -fix ./canal

In some cases I had to make sure the comments (that fieldalignment doesn't take into account) were restored.

@atercattus
Copy link
Member

Thank you for the efforts, fieldalign gives some interesting insights. But there is a fact that we need to respect maintainability and code readability too.

When rearranging fields, their logical grouping should be preserved.

We should not separate Host and Port fields here

	Host string
	Port uint16

VS

	Host string
	// a lot of strings ...
	Port uint16

The same goes for Tables+TablesDB and Tables+Databases here.
They should be near each other.

	// Will override Databases
	Tables  []string
	TableDB string

	Databases []string

VS

	TableDB string
	// ...
	Databases    []string
	ExtraOptions []string

	// Will override Databases
	Tables []string

And another example is a mutex field that is bundled with fields it protects here

	synchro struct {
		sync.Mutex
		// other fields
	}

VS

	synchro struct {
		// other fields
		sync.Mutex
		// other fields
	}

There are more examples, but I think 3 is enough to demonstrate the principle.

So, let's use fieldalign only when it doesn't break the code readability?

@skoef
Copy link
Contributor Author

skoef commented Jan 17, 2022

While converting certain structs I already imagined comments like this. I completely get the readability aspect, I will see if I can ignore the linter on specific structs that benefit from certain order in parameters.

@skoef skoef force-pushed the fieldalign branch 2 times, most recently from 203f28d to 4224d7d Compare January 17, 2022 14:10
password string
user string
salt []byte // should be 8 + 12 for auth-plugin-data-part-1 and auth-plugin-data-part-2
stmtID uint32
Copy link
Member

Choose a reason for hiding this comment

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

Could you keep stmts and stmtID near each other?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

While the config of binlog parsing shouldn't really put a hit on the memory usage when it isn't optimally aligned, especially server.Conn and client.Conn can make a real change in memory usage in a connection hungry application. I'd say this is the one struct you'd want to align as best as possible. I can add comments on what each field represents, perhaps that is the right middle ground?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@atercattus what is your opinion about this?

Copy link
Collaborator

@lance6716 lance6716 left a comment

Choose a reason for hiding this comment

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

I'm neutral to this PR 😄

@skoef
Copy link
Contributor Author

skoef commented Aug 26, 2022

let's just leave this as is for now

@skoef skoef closed this Aug 26, 2022
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