<div dir="ltr">Hi Alistair,<div class="gmail_extra"><br><div class="gmail_quote">On Mon, Jan 1, 2018 at 12:40 AM, Alistair Grant <span dir="ltr"><<a href="mailto:akgrant0710@gmail.com" target="_blank">akgrant0710@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex"><br>
Hi Eliot,<br>
<br>
I've posted a new version of the FileAttributesPlugin<br>
(FileAttributesPlugin.oscog-<wbr>AlistairGrant.19):<br>
<br>
<a href="http://smalltalkhub.com/#!/~Alistair/FileAttributesPlugin/versions/FileAttributesPlugin.oscog-AlistairGrant.19" rel="noreferrer" target="_blank">http://smalltalkhub.com/#!/~<wbr>Alistair/FileAttributesPlugin/<wbr>versions/FileAttributesPlugin.<wbr>oscog-AlistairGrant.19</a></blockquote><div><br></div><div>Looking good!  I have some minor quibbles, but so far see nothing major wrong.</div><div><br></div><div>So...</div><div><br></div><div>- 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</div><div><br></div><div>canStatFilePath: aPathCString length: length<br></div><div><span style="white-space:pre">  ...</span><br></div><div><span class="gmail-Apple-tab-span" style="white-space:pre">  </span>(hasSecurityPlugin = 0) ifTrue: [^ true].</div><div><span class="gmail-Apple-tab-span" style="white-space:pre">      </span>sCOFfn := interpreterProxy ioLoadFunction: 'secCanOpenFileOfSizeWritable' From: 'SecurityPlugin'.</div><div><span class="gmail-Apple-tab-span" style="white-space:pre">      </span>sCOFfn ~= 0</div><span style="white-space:pre">      ...</span></div><div class="gmail_quote"><span style="white-space:pre"><br></span></div><div class="gmail_quote"><span style="white-space:pre">These are minor:</span></div><div class="gmail_quote"><span style="white-space:pre"><br></span><div>- accessAttributesForFilename:into:startingAt: doesn't need the remapOop:in:.  No instantiations occur so the GC will never kick in.</div><div>- 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.</div><div>- in primitives such as primitiveFileExists I would rewrite</div><div><br></div><div><span class="gmail-Apple-tab-span" style="white-space:pre">      </span>accessFlag = 0</div><div><span class="gmail-Apple-tab-span" style="white-space:pre">         </span>ifTrue: [interpreterProxy pop: 2 thenPush: interpreterProxy trueObject]</div><div><span class="gmail-Apple-tab-span" style="white-space:pre">                </span>ifFalse: [interpreterProxy pop: 2 thenPush: interpreterProxy falseObject]</div><div><br></div><div>  as</div><div><br></div><div><span class="gmail-Apple-tab-span" style="white-space:pre">        i</span>nterpreterProxy pop: 2 thenPush: (accessFlag = 0</div><div><span class="gmail-Apple-tab-span" style="white-space:pre">      </span><span style="white-space:pre">     </span><span style="white-space:pre">     </span><span style="white-space:pre">     </span><span style="white-space:pre">     </span><span style="white-space:pre">     </span><span style="white-space:pre">     </span><span style="white-space:pre">     </span><span class="gmail-Apple-tab-span" style="white-space:pre">      </span>ifTrue: [iinterpreterProxy trueObject]</div><div><span class="gmail-Apple-tab-span" style="white-space:pre"> </span><span style="white-space:pre">     </span><span style="white-space:pre">     </span><span style="white-space:pre">     </span><span style="white-space:pre">     </span><span style="white-space:pre">     </span><span style="white-space:pre">     </span><span style="white-space:pre">     </span><span class="gmail-Apple-tab-span" style="white-space:pre">      </span>ifFalse: [interpreterProxy falseObject])</div><div><br></div><div>- 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.</div><div><br></div><div>- there are still some users of cPreprocessorDirective:.  It is much more preferable to use cppIf:ifTrue:[ifFalse:]</div><div><br></div><div><br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">Changes:<br>
<br>
- Convert error values from coded values to #primitiveFailForOSError:<br>
- #primitiveClosedir & #primitiveRewinddir return PrimErrBadArgument<br>
  instead of a coded error for a bad argument<br>
<br>
<br>
The updated Pharo code to use the modified plugin is in github:<br>
<br>
<a href="https://github.com/akgrant43/FileAttributes" rel="noreferrer" target="_blank">https://github.com/akgrant43/<wbr>FileAttributes</a><br>
<br>
Would you please review the plugin code again (taking in to account the<br>
feedback I sent in "Re: 20294 Add FileAttributesPlugin to the linux 32<br>
bit VM", sent 6 Sep 2017).<br>
<br>
<br>
Thanks!<br>
<span class="gmail-HOEnZb"><font color="#888888">Alistair<br>
</font></span><span class="gmail-im gmail-HOEnZb"><br>
<br>
<br>
On 23 December 2017 at 19:00, Eliot Miranda <<a href="mailto:eliot.miranda@gmail.com">eliot.miranda@gmail.com</a>> wrote:<br>
><br>
> Hi Alistair,<br>
><br>
</span><div class="gmail-HOEnZb"><div class="gmail-h5">> On Sat, Dec 23, 2017 at 9:35 AM, Alistair Grant <<a href="mailto:akgrant0710@gmail.com">akgrant0710@gmail.com</a>> wrote:<br>
>><br>
>><br>
>> Hi Eliot,<br>
>><br>
>> I've made progress:<br>
>><br>
>> - Updated FileAttributesPlugin to use #primitiveFailForOSError:.<br>
>> - Loaded PrimitiveOSError<br>
>> - Updated SmalltalkImage>><wbr>newSpecialObjectsArray<br>
>> - Run SmalltalkImage current recreateSpecialObjectsArray.<br>
>> - And triggered an error which generated the appropriate<br>
>> PrimitiveOSError object.<br>
>><br>
>> Given I've already made changes, and have a bit more tidying up to do,<br>
>> it is probably isn't worthwhile you reviewing the code again until I<br>
>> post an updated version.  I'll let you know when it is ready.<br>
><br>
><br>
> OK, that's great.  Thanks and have a great holiday!<br>
><br>
>><br>
>><br>
>> Merry Christmas!<br>
>> Alistair<br>
</div></div></blockquote></div><br><br clear="all"><div><br></div>-- <br><div class="gmail_signature"><div dir="ltr"><div><span style="font-size:small;border-collapse:separate"><div>_,,,^..^,,,_<br></div><div>best, Eliot</div></span></div></div></div>
</div></div>