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

Implement a well defined ordering among types #1718

Closed

Conversation

krader1961
Copy link
Contributor

Replace the current total ordering of types defined by the address at which each type is defined (which is effectively random) with an explicit ordering. This makes writing unit tests easier and provides predictable run time behavior for users regardless of which platform they run Elvish on.

Related #1495

Replace the current total ordering of types defined by the address at
which each type is defined (which is effectively random) with an
explicit ordering. This makes writing unit tests easier and provides
predictable run time behavior for users regardless of which platform
they run Elvish on.

Related elves#1495
@krader1961
Copy link
Contributor Author

I'll point out that my pending change to improve the pprint formatting of maps to vertically align values depends on this change. Primarily due to a unit test that deliberately includes mixed types to verify the formatting of the output is correct when mixed types are present. I appreciate why @xiaq rejected my previous solution to this problem but there really does need to be a platform independent ordering of heterogenous types. This change does so and is cheap. It also fixes a problem with the current implementation which does not treat regular and struct maps as equivalent when ordered. Not to mention simply making the order of heterogenous types consistent across platforms; rather than indeterminate.

@xiaq
Copy link
Member

xiaq commented Aug 23, 2023

Reflection is expensive, and the Name() is not unique; you need PkgPath() too.

A much cheaper option is to simply return small numbers:

func typeOf(x any) uintptr {
  switch x.(type) {
  case nil:
    return 0
  case bool:
    return 1
  // ...
  default:
    return *(*uintptr)(unsafe.Pointer(&x))
  }
}

I don't think Elvish supports any operating system that maps page 0 without an explicit mmap, but if there's a need to support such platforms, it's always possible to allocate a small byte array, and use its address instead:

var dummy [10]byte
var dummyBase = uintptr(unsafe.Pointer(&dummy))

func typeOf(x any) uintptr {
  switch x.(type) {
  case nil:
    return dummyBase
  // ...
}

However, I don't feel having predictable ordering for testing pprint is a strong enough reason for having the ordering of types fixed; it's not a real-world use case. I'll reconsider if there's a real-world use case.

For testing pprint: If there are two types, you can just list the two possible outputs. If there's really a need for many different types, you can build some test utility, like this (very rough sketch):

type reorderableLines struct{
  before string
  middle []string // can appear in any relative order
  after string
}

Test(t,
  That(`pprint $m`).Prints(reorderableLines{"[", []string{"&a=b\n&c=d", "&(num 1)=foo\n&(num 2)=bar"} "]"})

In any case, the logic of pprint should be agnostic about maps having mixed-typed keys, so I'm not sure that it's necessary to test in the first place. But maybe your implementation does care about that.

@xiaq xiaq closed this Aug 23, 2023
@xiaq
Copy link
Member

xiaq commented Aug 23, 2023

4ceb919 fixed the structmap bug.

@krader1961 krader1961 deleted the predictable-total-ordering branch January 27, 2024 07:04
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