[Vm-dev] Fwd: Merging FilesAttributesPlugin

[Alistair Grant]

Hi Eliot,

Thanks for your feedback!

TL;DR: I've made all the changes you suggested below (more verbose 
response below my signature).

FileAttributesPlugin.oscog-AlistairGrant.20 is now available at:

http://smalltalkhub.com/#!/~Alistair/FileAttributesPlugin/versions/FileAttributesPlugin.oscog-AlistairGrant.20

I'm not mentioning this every time, but there's about 540 automated 
tests related to the file system, which I confirm that all pass before 
posting updates.  I also use the plugin in my development environment.


Thanks again,
Alistair


On Mon, Jan 01, 2018 at 07:42:38PM -0800, Eliot Miranda wrote:
>  
> 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
> ...

Fixed.


> These are minor:
> 
> - accessAttributesForFilename:into:startingAt: doesn't need the remapOop:in:. 
> No instantiations occur so the GC will never kick in.

Done.


> - 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.

Done.


> - 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])

This follows from my self-talk: "If accessFlag is 0, then push true, 
otherwise, push false".

But I've changed them all anyway.


> - 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.

I've changed them all to use the accessor so that the simulator is 
easier later on.


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

Last time I tried this it generated invalid C code.  I think I know what 
I did wrong now, so #cPreprocessorDirective: has been replaced.  

Cheers,
Alistair