Skip to content

Add big-endian ints#83

Merged
xairy merged 2 commits into
google:masterfrom
xairy:big-endian
Oct 13, 2016
Merged

Add big-endian ints#83
xairy merged 2 commits into
google:masterfrom
xairy:big-endian

Conversation

@xairy

@xairy xairy commented Oct 11, 2016

Copy link
Copy Markdown
Contributor

No description provided.

Comment thread prog/prog.go Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

drop this

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread sysgen/sysgen.go Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

whenever I will see int8be in syscall description, I will say s/int8be/int8/
int8be is confusing, drop it

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread prog/prog.go Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

drop it along with int8be

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread prog/prog.go Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Let's give proper size to all types, including syscall arguments.
I don't see why int32be should be prohibited in syscall aguments, but allowed everywhere else. Moreover, syscall arguments do have size -- they are intptr's effectively. Sysgen already makes assumptions about intptr size in sz := int64(64) // TODO: assume that pointer is 8 bytes for now. Add const ptrSize = 8 to sysgen.go and use to give size to all types where size is 0 currently.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread sysgen/sysgen.go Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

lookupConst := ...
and drop the previous line

You may have seen this form with separate declaration and initialization in sys/prog, but it is necessary only when you have recursion (lookupConst would need to call lookupConst). Otherwise you just do f := func(.....

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

Comment thread sys/decl.go Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please let's do just 'BigEndian bool'.
This Endianness Endianness causes too much clutter for no apparent benefit. There are only 2 options, which is naturally represented by a bool. There are multiple copies

    Endianness: LittleEndian,

    Endianness == LittleEndian

    endianness := "LittleEndian"
    if !littleEndian {
        endianness = "BigEndian"
    }

If we do the bool, we can drop:

    endianness := "LittleEndian"
    if !littleEndian {
        endianness = "BigEndian"
    }

entirely, as a bool is correctly printed with %v.

We can also drop Endianness: LittleEndian, initialization, as default value for bool represents our semantic default of having little endian (unless said otherwise).

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@xairy

xairy commented Oct 13, 2016

Copy link
Copy Markdown
Contributor Author

PTAL

@dvyukov dvyukov left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Much nicer now. Thanks!
One small nit.

Comment thread prog/prog.go Outdated

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

s/value uintptr, size uintptr/value, size uintptr/

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Done.

@xairy

xairy commented Oct 13, 2016

Copy link
Copy Markdown
Contributor Author

PTAL

@dvyukov

dvyukov commented Oct 13, 2016

Copy link
Copy Markdown
Collaborator

LGTM

@xairy xairy merged commit a73bffc into google:master Oct 13, 2016
@xairy xairy deleted the big-endian branch October 13, 2016 13:42
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