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.

[Alex Ionescu]

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(a)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(a)videotron.ca>
>  *Sent:* Monday, September 05, 2011 8:25 PM
>  *To:* ReactOS Development List <ros-dev(a)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(a)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(a)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(a)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…
> > >
> > >  ==============================================================================
> > >  --- 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(a)reactos.org
> >  http://www.reactos.org/mailman/listinfo/ros-dev
> >
> >
> >  _______________________________________________
> >  Ros-dev mailing list
> >  Ros-dev(a)reactos.org
> >  http://www.reactos.org/mailman/listinfo/ros-dev
> >
> >  
>  ------------------------------
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>
>
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev
>
>