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 <marlowsd@gmail.com> 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
 <simonpj@microsoft.com <mailto:simonpj@microsoft.com>> 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:*michael.snoyman@gmail.com <mailto:michael.snoyman@gmail.com>
    [mailto:michael.snoyman@gmail.com
    <mailto:michael.snoyman@gmail.com>] *On Behalf Of *Michael Snoyman
    *Sent:* 02 November 2015 13:42


    *To:* Simon Peyton Jones
    *Cc:* ghc-devs@haskell.org <mailto:ghc-devs@haskell.org>
    *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
     <simonpj@microsoft.com <mailto:simonpj@microsoft.com>> 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:*michael.snoyman@gmail.com
        <mailto:michael.snoyman@gmail.com>
        [mailto:michael.snoyman@gmail.com
        <mailto:michael.snoyman@gmail.com>] *On Behalf Of *Michael Snoyman
        *Sent:* 02 November 2015 13:34
        *To:* Simon Peyton Jones
        *Cc:* ghc-devs@haskell.org <mailto:ghc-devs@haskell.org>
        *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
         <michael@snoyman.com <mailto:michael@snoyman.com>> 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
             <simonpj@microsoft.com <mailto:simonpj@microsoft.com>>
            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
                ghc-devs@haskell.org <mailto:ghc-devs@haskell.org>
                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
ghc-devs@haskell.org
http://mail.haskell.org/cgi-bin/mailman/listinfo/ghc-devs