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/FileAttri...
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@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