[ros-dev] [ros-diffs] [fireball] 53596: [NTOS/CONFIG] - Remove unnecessary assignments. Spotted by PVS and Dmitry Chapyshev. This may change the behaviour of that codepath, so test results are going to be observed.

Mon Sep 5 17:25:36 UTC 2011

Unless you are vicmarcal, my e-mail was not for you.

Best regards,
Alex Ionescu

On Mon, Sep 5, 2011 at 5:50 PM, Aleksey Bragin <aleksey at reactos.org> wrote:

>   Perfect, I have a straight answer from you right away, so my trick
> worked.
>
> Now, seriously, is that hack still needed? I understand it was needed
> during early time when you developed this branch, but what about now? With
> all the changes it had over time when you last worked on it?
>
>  *From:* Alex Ionescu <ionucu at videotron.ca>
> *Sent:* Monday, September 05, 2011 8:25 PM
> *To:* ReactOS Development List <ros-dev at reactos.org>
> *Subject:* Re: [ros-dev] [ros-diffs] [fireball] 53596: [NTOS/CONFIG] -
> Remove unnecessary assignments. Spotted by PVS and Dmitry Chapyshev. This
> may change the behaviour of that codepath, so test results are going to be
> observed.
>
> As a "non-dev" you seem to have understood the issue more than the project
> developer leader! Good job.
>
> Indeed, the original code lacks a comment stating that the first assignment
> is how it's "supposed" to be done, but that the second assignment is
> currently a hack to make ReactOS work (in the original commit I made, the
> first line was commented out -- but a comment never added as to why). So
> that's my fault.
>
> You're also right that this "unneccessary assignment" is actually quite
> necessary, and that by removing the SECOND ChildList assignment, instead of
> the first, the entire functionality of the function is modified -- if
> anything, the first assignment should've been removed (since no comment was
> there explaining why it was needed).
>
> So yes, good job on understanding both these issues, and for none of the
> devs seemingly able to see your correct logic...
>
> Best regards,
> Alex Ionescu
>
>
> On Mon, Sep 5, 2011 at 1:03 PM, Vicmarcal <vicmarcal at hotmail.com> wrote:
>
>>  Nope,I have asked in #reactos-dev too about that removal just after the
>> commiting was done.
>> To begin I dont understand the logic in the original code about
>> reassigning the value to the ChildList two lines later(unless the first
>> assignation is not needed at all)
>> And after the commit I dont understand   the commit message "unnecesary
>> assignments".Now,after removing the second assignment,the behavior should be
>> totally different(unless &Kcb->ValueCache; is == &KeyNode->ValueList; ,
>> which I doubt). So if the removal is correct,then it wasnt an "unnecesary
>> change" but a "fixing a bug".
>> Anyway I am not a dev, so I express my doubts here too if anyone can light
>> me a little :)
>>
>> Enviado desde mi iPhone
>>
>> El 05/09/2011, a las 13:48, "Alex Ionescu" <ionucu at videotron.ca>
>> escribió:
>>
>>    Uhhhh...
>>
>> Am I really the *only* one who sees a problem here?
>>
>> ChildList = &Kcb->ValueCache;
>> -    ChildList = (PCACHED_CHILD_LIST)&KeyNode->ValueList;
>>
>> Best regards,
>> Alex Ionescu
>>
>>
>> On Mon, Sep 5, 2011 at 10:54 AM, <fireball at svn.reactos.org> wrote:
>>
>>> Author: fireball
>>> Date: Mon Sep  5 09:54:20 2011
>>> New Revision: 53596
>>>
>>> URL: http://svn.reactos.org/svn/reactos?rev=53596&view=rev
>>> Log:
>>> [NTOS/CONFIG]
>>> - Remove unnecessary assignments. Spotted by PVS and Dmitry Chapyshev.
>>> This may change the behaviour of that codepath, so test results are going to
>>> be observed.
>>>
>>> Modified:
>>>    trunk/reactos/ntoskrnl/config/cmvalche.c
>>>
>>> Modified: trunk/reactos/ntoskrnl/config/cmvalche.c
>>> URL:
>>> http://svn.reactos.org/svn/reactos/trunk/reactos/ntoskrnl/config/cmvalche.c?rev=53596&r1=53595&r2=53596&view=diff
>>>
>>> ==============================================================================
>>> --- trunk/reactos/ntoskrnl/config/cmvalche.c [iso-8859-1] (original)
>>> +++ trunk/reactos/ntoskrnl/config/cmvalche.c [iso-8859-1] Mon Sep  5
>>> 09:54:20 2011
>>> @@ -49,7 +49,6 @@
>>>     PHHIVE Hive;
>>>     PCACHED_CHILD_LIST ChildList;
>>>     HCELL_INDEX CellToRelease;
>>> -    PCM_KEY_NODE KeyNode;
>>>
>>>     /* Set defaults */
>>>     *ValueListToRelease = HCELL_NIL;
>>> @@ -58,8 +57,6 @@
>>>     /* Get the hive and value cache */
>>>     Hive = Kcb->KeyHive;
>>>     ChildList = &Kcb->ValueCache;
>>> -    KeyNode = (PCM_KEY_NODE)HvGetCell(Hive, Kcb->KeyCell);
>>> -    ChildList = (PCACHED_CHILD_LIST)&KeyNode->ValueList;
>>>
>>>     /* Check if the value is cached */
>>>     if (CmpIsValueCached(ChildList->ValueList))
>>> @@ -212,7 +209,6 @@
>>>     BOOLEAN IndexIsCached;
>>>     ULONG i = 0;
>>>     HCELL_INDEX Cell = HCELL_NIL;
>>> -    PCM_KEY_NODE KeyNode;
>>>
>>>     /* Set defaults */
>>>     *CellToRelease = HCELL_NIL;
>>> @@ -221,8 +217,6 @@
>>>     /* Get the hive and child list */
>>>     Hive = Kcb->KeyHive;
>>>     ChildList = &Kcb->ValueCache;
>>> -    KeyNode = (PCM_KEY_NODE)HvGetCell(Hive, Kcb->KeyCell);
>>> -    ChildList = (PCACHED_CHILD_LIST)&KeyNode->ValueList;
>>>
>>>     /* Check if the child list has any entries */
>>>     if (ChildList->Count != 0)
>>>
>>>
>>>
>>
>>  _______________________________________________
>> Ros-dev mailing list
>> Ros-dev at reactos.org
>> http://www.reactos.org/mailman/listinfo/ros-dev
>>
>>
>> _______________________________________________
>> Ros-dev mailing list
>> Ros-dev at reactos.org
>> http://www.reactos.org/mailman/listinfo/ros-dev
>>
>>
>
> ------------------------------
> _______________________________________________
> Ros-dev mailing list
> Ros-dev at reactos.org
> http://www.reactos.org/mailman/listinfo/ros-dev
>
>
> _______________________________________________
> Ros-dev mailing list
> Ros-dev at reactos.org
> http://www.reactos.org/mailman/listinfo/ros-dev
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://www.reactos.org/pipermail/ros-dev/attachments/20110905/4343f36c/attachment-0001.htm>