[Vm-dev] Fwd: Merging FilesAttributesPlugin

[Eliot Miranda]

Hi Alistair,

On Mon, Jan 1, 2018 at 12:40 AM, Alistair Grant <akgrant0710 at gmail.com>
wrote:

>
> Hi Eliot,
>
> I've posted a new version of the FileAttributesPlugin
> (FileAttributesPlugin.oscog-AlistairGrant.19):
>
> http://smalltalkhub.com/#!/~Alistair/FileAttributesPlugin/
> versions/FileAttributesPlugin.oscog-AlistairGrant.19


Looking good!  I have some minor quibbles, but so far see nothing major
wrong.

So...

- the most important is not to query the SecurityPlugin more than once.  So
follow the pattern in FilePlugin where the inst vars such as sCOFfn are set
up in initialiseModule.  You're querying the SecurityPlugin on every access
as in

canStatFilePath: aPathCString length: length
...
(hasSecurityPlugin = 0) ifTrue: [^ true].
sCOFfn := interpreterProxy ioLoadFunction: 'secCanOpenFileOfSizeWritable'
From: 'SecurityPlugin'.
sCOFfn ~= 0
...

These are minor:

- accessAttributesForFilename:into:startingAt: doesn't need the
remapOop:in:.  No instantiations occur so the GC will never kick in.
- primitives such as primitiveClosedir that use pointerFor: don't need to
check the argument via is: dirPointerOop KindOf: 'ByteArray'; pointerFor:
does all the checking one needs.
- in primitives such as primitiveFileExists I would rewrite

accessFlag = 0
ifTrue: [interpreterProxy pop: 2 thenPush: interpreterProxy trueObject]
ifFalse: [interpreterProxy pop: 2 thenPush: interpreterProxy falseObject]

  as

interpreterProxy pop: 2 thenPush: (accessFlag = 0
ifTrue: [iinterpreterProxy trueObject]
ifFalse: [interpreterProxy falseObject])

- given that you're accessing W_OK et al via Symbols (see #W_OK et al in
accessAttributesForFilename:into:startingAt:) do you need fileWriteableFlag
et al?  Or should you use them instead of the Symbols?  The latter is
easier when we have to simulate the plugin when debugging the VM at some
future date.

- there are still some users of cPreprocessorDirective:.  It is much more
preferable to use cppIf:ifTrue:[ifFalse:]


Changes:
>
> - Convert error values from coded values to #primitiveFailForOSError:
> - #primitiveClosedir & #primitiveRewinddir return PrimErrBadArgument
>   instead of a coded error for a bad argument
>
>
> The updated Pharo code to use the modified plugin is in github:
>
> https://github.com/akgrant43/FileAttributes
>
> Would you please review the plugin code again (taking in to account the
> feedback I sent in "Re: 20294 Add FileAttributesPlugin to the linux 32
> bit VM", sent 6 Sep 2017).
>
>
> Thanks!
> Alistair
>
>
>
> On 23 December 2017 at 19:00, Eliot Miranda <eliot.miranda at gmail.com>
> wrote:
> >
> > Hi Alistair,
> >
> > On Sat, Dec 23, 2017 at 9:35 AM, Alistair Grant <akgrant0710 at gmail.com>
> wrote:
> >>
> >>
> >> Hi Eliot,
> >>
> >> I've made progress:
> >>
> >> - Updated FileAttributesPlugin to use #primitiveFailForOSError:.
> >> - Loaded PrimitiveOSError
> >> - Updated SmalltalkImage>>newSpecialObjectsArray
> >> - Run SmalltalkImage current recreateSpecialObjectsArray.
> >> - And triggered an error which generated the appropriate
> >> PrimitiveOSError object.
> >>
> >> Given I've already made changes, and have a bit more tidying up to do,
> >> it is probably isn't worthwhile you reviewing the code again until I
> >> post an updated version.  I'll let you know when it is ready.
> >
> >
> > OK, that's great.  Thanks and have a great holiday!
> >
> >>
> >>
> >> Merry Christmas!
> >> Alistair
>



-- 
_,,,^..^,,,_
best, Eliot
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.squeakfoundation.org/pipermail/vm-dev/attachments/20180101/2f574f14/attachment.html>