I'd rather not change the public API (even though it's an internal module) in a way we'll later undo to avoid a warning, when the code cleanup should achieve the goal without making that modification. I've pushed my changes to the less-cpp branch, if anyone wants to play with the changes thus far. I've also modified the Travis build to use -Wall -Werror, and added an AppVeyor Windows build that similarly uses -Werror.
On Mon, Nov 2, 2015 at 6:44 AM, Simon Marlow <[email protected]> wrote: > -Wwarn shouldn't really be in source code. Since it's an Internals > module, I would just export it. Maybe add a {-# WARNING #-} to discourage > people from using it. > > Cheers > Simon > > On 02/11/2015 06:14, Michael Snoyman wrote: > >> >> >> On Mon, Nov 2, 2015 at 5:57 AM, Simon Peyton Jones >> <[email protected] <mailto:[email protected]>> wrote: >> >> Aha. It would be great to say all that in the source code!! It’s >> very non-obvious that you not want people ever to construct a CGId >> on Windows. After all, it has a newtype definition!____ >> >> __ >> >> >> Good call, I'll update with some comments (though see refactoring >> comments below). >> >> __ >> >> Could you declare it differently?____ >> >> data CGId -- No constructors____ >> >> __ >> >> >> Certainly we could. Then the question would be "why does the Windows >> code look so different?" There are lots of colors to this bikeshed, and >> I don't have any particular affinity to any of them. If you'd prefer it >> looked that way, I can make that change. I initially erred with making >> the code as similar to the POSIX code as possible. >> >> __ >> >> Also if so, why does the Windows-specific foreign import of >> c_runInteractiveProcess (lin 440-is) pas a Ptr CGId? You’d just told >> me that we can never create one.____ >> >> __ >> >> >> >> The Windows-specific foreign import is on line ~533, and does not >> include those arguments. >> >> __ >> >> Also, ____ >> >> __·__It’d help to comment the #else on line 456 as being “else if >> not windows”____ >> >> __·__The #endifs on line 546 or thereabouts are mis-labelled. at >> the moment the second says “GLASGOW_HASKELL” but actually it’s the >> first____ >> >> __ >> >> >> >> Agreed that the code is fairly difficult to follow with the nested >> ifdefs. However, instead of trying to salvage that, it's probably worth >> a refactoring to put the Windows and POSIX code into separate modules >> and then just import those conditionally. >> >> __ >> >> I have no opinion about the best solution; I’d just like it to >> compile and preferably warning free since that is our default >> policy. Or add a –Wwarn at the top.____ >> >> __ >> >> >> If you're looking for a short-term solution, I can add -Wwarn to the top >> until some kind of refactoring takes place. >> >> __ >> >> thanks____ >> >> __ __ >> >> Simon____ >> >> __ __ >> >> __ __ >> >> *From:*[email protected] <mailto:[email protected]> >> [mailto:[email protected] >> <mailto:[email protected]>] *On Behalf Of *Michael Snoyman >> *Sent:* 02 November 2015 13:42 >> >> >> *To:* Simon Peyton Jones >> *Cc:* [email protected] <mailto:[email protected]> >> *Subject:* Re: process library broken on Windows____ >> >> __ __ >> >> That's the goal; it's a feature that does not work on Windows, only >> on non-Windows systems (setuid/setgid for a child process). For >> POSIX systems, CGid is exported from base, and can be used. On >> Windows, the data type is present to give the same signature, but >> the constructor itself is not exported to prevent using the feature. >> An argument could be made for other approaches:____ >> >> __ __ >> >> 1. Expose the constructor, allowing users to set a value, and that >> value will be ignored____ >> >> 2. Make the fields themselves conditional on the OS being used____ >> >> __ __ >> >> I don't think there's a strong argument in any direction for this.____ >> >> __ __ >> >> On Mon, Nov 2, 2015 at 5:37 AM, Simon Peyton Jones >> <[email protected] <mailto:[email protected]>> wrote:____ >> >> I’m puzzled. Internals.hs defines a newtype____ >> >> ____ >> >> newtype CGid = CGid Word32____ >> >> ____ >> >> A value of this type is needed to fill in the child_group field >> of CreateProcess. If you don’t export it, you could never >> initialise this field to anything other than Nothing, so why do >> you have it?____ >> >> ____ >> >> Looks to me as if the warning has nailed a real bug____ >> >> ____ >> >> Simon____ >> >> ____ >> >> *From:*[email protected] >> <mailto:[email protected]> >> [mailto:[email protected] >> <mailto:[email protected]>] *On Behalf Of *Michael >> Snoyman >> *Sent:* 02 November 2015 13:34 >> *To:* Simon Peyton Jones >> *Cc:* [email protected] <mailto:[email protected]> >> *Subject:* Re: process library broken on Windows____ >> >> ____ >> >> I didn't read closely enough: I see now that it's a warning, not >> an error. I initially didn't export that constructor since it's >> only present on Windows for API compatibility, but will never be >> used. Since this is just the internals module, I can export it, >> but my preference would in fact be to leave it as-is with the >> warning. Two alternatives:____ >> >> ____ >> >> 1. Create a new hidden module that creates and exports the type >> constructor, just to hide the warning. I'm -1 on that, since >> that's extra compile time everyone has to endure just for >> warning avoidance.____ >> >> 2. base could export CGid for Windows (currently, it does >> not).____ >> >> ____ >> >> On Mon, Nov 2, 2015 at 5:17 AM, Michael Snoyman >> <[email protected] <mailto:[email protected]>> wrote:____ >> >> I'll look into this. I just made a new release of process, >> and was certain I tested on Windows, but perhaps something >> changed between that commit and release.____ >> >> ____ >> >> On Mon, Nov 2, 2015 at 5:15 AM, Simon Peyton Jones >> <[email protected] <mailto:[email protected]>> >> wrote:____ >> >> I’m getting this on HEAD in te ‘____ >> >> libraries\process\System\Process\Internals.hs:106:16: >> warning:____ >> >> Defined but not used: data constructor ‘CGid’____ >> >> Indeed it looks as if CGId(..) should be exported, else >> createProcess is unusuable. This looks like the right >> change. Would someone like to check and make the >> change____ >> >> Simon____ >> >> diff --git a/System/Process/Internals.hs >> b/System/Process/Internals.hs____ >> >> index 5575ac4..3e23ad5 100644____ >> >> --- a/System/Process/Internals.hs____ >> >> +++ b/System/Process/Internals.hs____ >> >> @@ -37,6 +37,8 @@ module System.Process.Internals (____ >> >> #if !defined(mingw32_HOST_OS) && !defined(__MINGW32__)____ >> >> pPrPr_disableITimers, c_execvpe,____ >> >> ignoreSignal, defaultSignal,____ >> >> +#else____ >> >> + CGid(..), GroupID, UserID,____ >> >> #endif____ >> >> withFilePathException, withCEnvironment,____ >> >> translate,____ >> >> ____ >> >> _______________________________________________ >> ghc-devs mailing list >> [email protected] <mailto:[email protected]> >> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs >> < >> https://na01.safelinks.protection.outlook.com/?url=http%3a%2f%2fmail.haskell.org%2fcgi-bin%2fmailman%2flistinfo%2fghc-devs&data=01%7c01%7csimonpj%40064d.mgd.microsoft.com%7cdd25a0d693de489171bb08d2e38a505d%7c72f988bf86f141af91ab2d7cd011db47%7c1&sdata=YdqpMC5wr2K6oUOw9WImRGpSl6EsV8zQyAK%2ba26oF9M%3d >> >____ >> >> ____ >> >> ____ >> >> __ __ >> >> >> >> >> _______________________________________________ >> ghc-devs mailing list >> [email protected] >> http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs >> >>
_______________________________________________ ghc-devs mailing list [email protected] http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs
