Implement BaudRate with PatternSynonyms. #145
Conversation
|
@hvr curious to hear your thoughts. |
|
While this is definitely an elegant way the big downside here is that this would require PatternSynonyms from the compiler, which cuts off everything before GHC 7.8 (supporting GHC 7.8 will require some extra work btw; the PatternSynonyms impl was improved in GHC 7.10 which renders the current PR incompatible with GHC 7.8) :-/ See https://hackage.haskell.org/package/unix to get an idea of unix' track-record of backward compat. I'm not sure if this API refinement would justify breaking compatibility with those compilers; but if we do this, we can rethink other places as well to take advantage of newer GHCs -- then there's a lot more to break to make this count! |
Which shouldn't block this PR though. |
|
This is blocked, until we decide to abandon support for GHC < 8.0. @TravisWhitaker would you like to make a case why the proposed approach is better than status quo? |
I worked extensively with Kazu Yamamoto in the Pattern synonyms are definitely the way to go when the underlying enumerated type is "open", in the sense that new member elements are added from time to time, or existing member elements are platform-specific. The pattern synonyms are also more efficient, since they directly capture the underlying numeric values passed to system calls, ... New releases of So I would strongly advocate for dropping support for GHC 7.x as soon as absolutely possible. The older releases are no longer maintained. I just filed MRs fixing a critical issue in the 8.8,8.10,9.0 and master MVar versions, but the same bug is present in GHC 8.6 and at least as far back as 7.8. These GHC versions are abandonware, and I see no reason to continue to support them in updated major releases of packages. Any users clinging to GHC 7.x can use corresponding older releases of packages they need. |
| @@ -48,7 +49,100 @@ module System.Posix.Terminal.Common ( | |||
| minInput, | |||
| withMinInput, | |||
|
|
|||
| BaudRate(..), | |||
| BaudRate, | |||
There was a problem hiding this comment.
I'd like to see this export list maintained in just one module. Perhaps System.Postfix.Terminal.Exports with Common and ByteString both re-exporting Exports or something along these lines...
There was a problem hiding this comment.
Seems fine to me, but what's the rationale?
| #ifdef B0 | ||
| pattern B0 :: BaudRate | ||
| pattern B0 = BaudRate (#const B0) | ||
| #endif |
There was a problem hiding this comment.
Here and elsewhere I'd like to see the baud rates that are not supported on the current platform mapped to a known unused value, likely -1 will work, ... so that the well-known values are defined on all platforms, even if not actually mapped to usable baud rate.
The sentinel value should also have an explicit name (UnsupportedBaudRate or similar), which allows users to check whether the baud-rate they are looking for is supported or not.
There was a problem hiding this comment.
A sentinel value works much better than bottom
There was a problem hiding this comment.
What do you think is defined to be bottom here? On a given platform, the undefined baud rate macros' patterns are also undefined.
There was a problem hiding this comment.
My point is that none of the values should be "error", they should instead be a non-error sentinel, that one can compare a given value with to determine whether it is supported or not. The unsupported value can probably be (-1).
There was a problem hiding this comment.
With this PR, none of the values are "error."
There was a problem hiding this comment.
But the patterns should be defined unconditionally, even when the OS does not have a define for the name, in which case the name should map to a sentinel value that indicates an invalid Baud rate.
There was a problem hiding this comment.
Why is that better than the behavior one gets with C, where using an undefined baud macro would yield a compile time error instead?
There was a problem hiding this comment.
Why is that better than the behavior one gets with C, where using an undefined baud macro would yield a compile time error instead?
Because, in an actual C application, the call site would have an #Ifdef the particular macro, and the code would compile on all platforms! Here, we're providing a Haskell module that abstracts the C interface, but the caller is not expected to have feature-test CPP macros in their code, that's our job. And yet the caller's code must compile on all platforms.
Therefore, we must present the same set of pattern synonyms on all platforms, and allow run-time discovery of which ones are supported by the platform.
| #else | ||
| baud2Word B4000000 = error "B4000000 not available on this system" | ||
| #endif | ||
| baud2Word = coerce | ||
|
|
||
| -- And convert a word back to a baud rate | ||
| -- We really need some cpp macros here. | ||
|
|
||
| word2Baud :: CSpeed -> BaudRate | ||
| word2Baud x = case x of |
There was a problem hiding this comment.
This function should accept any number, whether known at the time the package was released, or not, and should not throw errors by default. If the value is not actually supported by the terminal driver, the user will find out when the relevant ioctl fails.
There was a problem hiding this comment.
We should however also provide sensible Show and Read instances for these, examples can be found in the network package.
There was a problem hiding this comment.
With #175 merged, this PR can resume progress. But we do need to sort out my prior comments. Are you ready/willing to get this back on track? If not, I could perhaps take it over and get it across the line?
|
Is this missing the |
|
Closing, replaced by #216 |
This is a sketch of a new way of handling baud rates. The idea is to make
BaudRatean opaque type, and provide pattern synonyms based on the availability of baud rate macros from the local termios header. There are two downsides to this approach:-XPatternSynonyms.BaudRateis to usebaud2Word.I'm responsible for lots of Haskell code wrapping C APIs, and this approach seems to work well. The gl package uses the same approach for handling constants from C headers.