Communication Problems re 47514 & co.

[Ros Arm]

Hello,

First I was asked if a certain developer could talk to me regarding ARM3. I said yes,
never heard back, and then saw commits done to pfnlist.c. While interesting and useful
code, some communication would've been nice in order to talk about the goals, since
much of that code is in flux (I feel bad when someone improves a function that I will
later remove, as two of the improved functions are only temporary -- the changes to the
other functions will remain, obviously). Normally, this would not have been a problem, if
not for the strange formatting and the fact this code was now broken and causing a boot
regression.

I showed up in the #reactos-dev channel and instructed developers on how to fix the
problem: remove the entire function and use MiInsertPageInFreeList instead, or fix
MiInitializePagingLists to perform its work at DISPATCH_LEVEL instead, by acquiring the
PFN lock as it should do.

None of this was done, instead "janderwald", in 47514, "Fixed" the
ASSERT by saying "Any IRQL lower than DISPATCH_LEVEL" is okay. Janderwald,
whoever you are, do you even understand this code? Are you even a kernel-mode developer?
Does it make to you that the code would need to assert for low IRQL? The whole point is to
validate that the PFN lock is being held. Even if you are not a kernel-mode dev, isn't
it clear that all the other assertions are also checking for DISPATCH_LEVEL+?

And even if that's not clear, I had written down the solution in the #reactos-dev
channel, expecting that probably not many people would figure it out.

So I guess some lessons to learn for this project:

1) If you modify someone's code, make sure you test your changes (and preferably
don't mess up the formatting) so their code is not now blamed for a boot regression.
2) If someone tells you how to fix their code after it's been messed up, apply the
fix.
3) If you choose to roll your own fix, write something useful instead of breaking the
function (logic-wise) even more.

Again, I'm afraid the problem is most of you are students and don't have work
ethics of real employees -- this seems to be the root of many problems this project has
had since the last 2 years I've been watching and working on the ARM port.

Next steps:

- Janderwald: remove your ASSERT hack. Either 1) Remove the ASSERT (stop checking for an
invariant that doesn't happen) or 2) Fix the caller (so the invariant holds)
- Arthur: changes are appreciated, but let's talk more so you don't end up
improving dead code.

Carry on!

-r

[Ros Arm]

Hello,

Thank you Cameron (47516). +1!

Carry on!

-r

>   Hello,
>  
>  First I was asked if a certain developer could talk to me regarding ARM3. I said yes,
> never heard back, and then saw commits done to pfnlist.c. While interesting and useful
> code, some communication would've been nice in order to talk about the goals, since
> much of that code is in flux (I feel bad when someone improves a function that I will
> later remove, as two of the improved functions are only temporary -- the changes to the
> other functions will remain, obviously). Normally, this would not have been a problem, if
> not for the strange formatting and the fact this code was now broken and causing a boot
> regression.
>  
>  I showed up in the #reactos-dev channel and instructed developers on how to fix the
> problem: remove the entire function and use MiInsertPageInFreeList instead, or fix
> MiInitializePagingLists to perform its work at DISPATCH_LEVEL instead, by acquiring the
> PFN lock as it should do.
>  
>  None of this was done, instead "janderwald", in 47514, "Fixed" the
> ASSERT by saying "Any IRQL lower than DISPATCH_LEVEL" is okay. Janderwald,
> whoever you are, do you even understand this code? Are you even a kernel-mode developer?
> Does it make to you that the code would need to assert for low IRQL? The whole point is to
> validate that the PFN lock is being held. Even if you are not a kernel-mode dev, isn't
> it clear that all the other assertions are also checking for DISPATCH_LEVEL+?
>  
>  And even if that's not clear, I had written down the solution in the #reactos-dev
> channel, expecting that probably not many people would figure it out.
>  
>  So I guess some lessons to learn for this project:
>  
>  1) If you modify someone's code, make sure you test your changes (and preferably
> don't mess up the formatting) so their code is not now blamed for a boot regression.
>  2) If someone tells you how to fix their code after it's been messed up, apply the
> fix.
>  3) If you choose to roll your own fix, write something useful instead of breaking the
> function (logic-wise) even more.
>  
>  Again, I'm afraid the problem is most of you are students and don't have work
> ethics of real employees -- this seems to be the root of many problems this project has
> had since the last 2 years I've been watching and working on the ARM port.
>  
>  Next steps:
>  
>  - Janderwald: remove your ASSERT hack. Either 1) Remove the ASSERT (stop checking for an
> invariant that doesn't happen) or 2) Fix the caller (so the invariant holds)
>  - Arthur: changes are appreciated, but let's talk more so you don't end up
> improving dead code.
>  
>  Carry on!
>  
>  -r
>  
>  _______________________________________________
>  Ros-dev mailing list
>  Ros-dev(a)reactos.org
>  http://www.reactos.org/mailman/listinfo/ros-dev

[Olaf Siejka]

Bit more on real working/coding ethics:

http://stackoverflow.com/questions/184618/what-is-the-best-comment-in-sourc…

2010/6/1 Ros Arm <ros.arm(a)reactos.org>

>   Hello,
>  So I guess some lessons to learn for this project:
>
>  1) If you modify someone's code, make sure you test your changes (and
>  preferably don't mess up the formatting) so their code is not now blamed for
>  a boot regression.
>  2) If someone tells you how to fix their code after it's been messed up,
>  apply the fix.
>  3) If you choose to roll your own fix, write something useful instead of
>  breaking the function (logic-wise) even more.
>
>  Again, I'm afraid the problem is most of you are students and don't have
>  work ethics of real employees -- this seems to be the root of many problems
>  this project has had since the last 2 years I've been watching and working
>  on the ARM port.
>
>   Carry on!
>
>  -r
>