metrics/generic: fix uint64 alignment#1007
Merged
peterbourgon merged 2 commits intogo-kit:masterfrom Sep 15, 2020
Merged
Conversation
ldez
force-pushed
the
fix/unit64aligment
branch
from
ed1ab96 to
093af6f
Compare
ldez
changed the title
ldez
force-pushed
the
fix/unit64aligment
branch
2 times, most recently
from
74f16a5 to
b138422
Compare
Member
|
Sure! Please add a test :) |
Contributor
Author
|
🤔 sure but could you help me?
I don't write this kind of test every day so any help would be welcome. |
ldez
force-pushed
the
fix/unit64aligment
branch
from
b138422 to
aec653a
Compare
Contributor
Author
|
@peterbourgon friendly ping 😉 |
Member
|
Sorry, I don't know offhand!
…On Fri, Sep 4, 2020 at 11:11 AM Ludovic Fernandez ***@***.***> wrote:
@peterbourgon <https://github.com/peterbourgon> friendly ping 😉
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#1007 (comment)>, or
unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA36TLOD4F6AMFECL45YSLSECVMVANCNFSM4QJPZKTA>
.
|
Contributor
Author
|
Without a 32bit arch it's not possible to create a simple test about alignment problem on a 32bits arch. I see only one solution: be able to run the tests of the project on a CI that use a 32bit arch. In all cases, this kind of change of the CI seems out of the scope of my PR. As a kind of prof, I will give you some dumps. On a 32bit archCreated from an old PI2-B (arm) Previous version:the My version:On a 64bit archPrevious version:My version: |
Member
Well, it's important to the long-term maintainability of the project that changes come with regression tests, so I don't accidentally undo your work in a future refactor. Can you think of any other way to accomplish that? |
Contributor
Author
|
I put a comment on the structure to explain why the field should be placed at the top of the structure. |
ldez
force-pushed
the
fix/unit64aligment
branch
from
aec653a to
12171c7
Compare
Contributor
Author
|
I added a test, but it's a weak and complex test, the maintainability is low, because the real solution is to run the tests on a 32-bit arch. FYI even staticcheck is not able to catch this misalignment properly without a 32-bit arch. Currently, I don't know any linter that can properly check this without a 32-bit arch. |
ldez
force-pushed
the
fix/unit64aligment
branch
from
12171c7 to
0250207
Compare
ldez
force-pushed
the
fix/unit64aligment
branch
from
0250207 to
aec4993
Compare
Contributor
Author
|
@peterbourgon friendly ping 😉 |
Member
|
Wow that is an... astonishing test ;) OK, I'm happy with this. Final question, though — is there a command I could run on the relevant architectures that would return one result if the alignment is "good" and a different result if the alignment was "bad"? |
Contributor
Author
|
As I explained in the code comment and in my previous message, you just have to run the existing tests or : package main
import (
"fmt"
"github.com/go-kit/kit/metrics/generic"
)
func main() {
counter := generic.Counter{}
counter.Add(6)
fmt.Println(counter)
}go test ./... (on ARM)If you are asking for a linter or something like that: no linter is perfect and this problem is complex, so I can not recommend a tool. |
Contributor
Author
|
@peterbourgon ping 🙏 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes uint64 alignment on 32-bit arch.
panic on Counter
panic on Gauge
Related to traefik/traefik#7207