Add 'fillBytes' to Foreign.Marshal.Utils.
ClosedPublic

Authored by ifesdjeen on Nov 9 2014, 2:26 PM.

Details

Summary

fillBytes uses 'memset' to fill a memory area with a required byte value.

I've made all the changes requested by @duncan in https://phabricator.haskell.org/D461:

  • moved the change to Foreign.Marshal.Utils
  • fixed the import location (string.h instead of stdlib.h)
  • fixed the import types (Word8 -> Int instead of previous CInt -> CSize), types were used in accordance with copyBytes interface
  • comment was improved as per @duncan's advise

(all the reviewers from #D461 are preserved)

Diff Detail

Repository
rGHC Glasgow Haskell Compiler
Branch
feature/malloc
Lint
Lint WarningsExcuse: Keeping a style of 'memcpy' and 'memmove'
SeverityLocationCodeMessage
Warninglibraries/base/Foreign/Marshal/Utils.hs:186TXT3Line Too Long
Unit
No Unit Test Coverage
Build Status
Buildable 1836
Build 1843: GHC Patch Validation (amd64/Linux)
ifesdjeen updated this revision to Diff 1364.Nov 9 2014, 2:26 PM
ifesdjeen retitled this revision from to Add 'fillBytes' to Foreign.Marshal.Utils..
ifesdjeen updated this object.
ifesdjeen edited the test plan for this revision. (Show Details)
ifesdjeen updated this object.Nov 9 2014, 2:30 PM
ifesdjeen edited edge metadata.
ifesdjeen updated this object.
ifesdjeen added reviewers: simonmar, duncan.
ifesdjeen added a subscriber: duncan.
ifesdjeen updated this revision to Diff 1365.Nov 9 2014, 3:27 PM

Added an uncommited Word8 import

hvr added a reviewer: ekmett.Nov 10 2014, 2:08 AM
ifesdjeen updated this revision to Diff 1372.Nov 10 2014, 2:19 AM

Remove incorrectly placed 8 from "Data.Word" import.

ifesdjeen updated this revision to Diff 1373.Nov 10 2014, 2:43 AM

Add an uncommited CInt import

Hi Everyone,

Sorry to disturb you, do you think the change is reasonable? Do you think you could merge it?

As soon as that one's done, I'll send a patch with @duncan's idea with malloc0, I already have a prototype, implemented it in my FFI tutorial.

hvr edited edge metadata.Nov 12 2014, 8:12 AM

Sorry to disturb you, do you think the change is reasonable? Do you think you could merge it?

@ifesdjeen, as noted in D461#11545, this should undergo discussion on the libraries@ mailing list before we merge it

Ah, perfect. I've filed a proposal to libraries@ mailing list.

austin edited edge metadata.Nov 20 2014, 11:56 AM

FWIW, I think this is fine and goes well with the other functions, so I approve.

austin accepted this revision.Nov 20 2014, 12:02 PM
austin edited edge metadata.
This revision is now accepted and ready to land.Nov 20 2014, 12:02 PM
hvr requested changes to this revision.Nov 20 2014, 12:26 PM
hvr edited edge metadata.

generally LGTM; got only two doc-related requests:

  • Please add an entry to the libraries/base/changelog.md file
libraries/base/Foreign/Marshal/Utils.hs
173

Please add an additional -- /Since: 4.8.0.0/ as last-line

174

(I'm not sure if this was discussed: is that the most obvious/consistent/natural argument-ordering?)

This revision now requires changes to proceed.Nov 20 2014, 12:26 PM
ifesdjeen updated this revision to Diff 1594.Nov 20 2014, 12:41 PM
ifesdjeen edited edge metadata.

Add 'fillBytes' to Foreign.Marshal.Utils.

fillBytes uses 'memset' to fill a memory area with
a required byte value.

@hvr I've updated changelog.md and added Since into comment block.

As regards the ordering, this one matches the original, and works pretty much the same way as the others (memcpy and memmove). I personally have used it in my project internally for a week or more and it was quite intuitive for me. Do you have objections?

austin accepted this revision.Nov 20 2014, 10:29 PM
austin edited edge metadata.

LGTM. I think it's fine: it's consistent with the C API for memset.

hvr accepted this revision.Nov 21 2014, 8:18 AM
hvr edited edge metadata.

As regards the ordering,[...]. Do you have objections?

No, no objections. I just wanted to make sure that ordering was chosen deliberately.

libraries/base/changelog.md
103 ↗(On Diff #1594)

Minor issue: Those aren't backquotes, which means this won't be rendered as monospace font

This revision is now accepted and ready to land.Nov 21 2014, 8:18 AM
This revision was automatically updated to reflect the committed changes.