Re: [Patch v2 1/2] UefiCpuPkg/RegisterCpuFeaturesLib: Combine implementation.

[Laszlo Ersek <lersek@redhat.com>]

On 08/09/18 05:18, Ni, Ruiyu wrote:

> We've reviewed the whole producer/consumer code and came to the conclusion
> that 4GB/NVS restriction is unnecessary.

I'd like to comment on this from a workflow POV as well.

If you perform a detailed review of the code, that's great. However, in
that case, please do take the time to *document* the review *in detail*
in the commit message.

I assume you and Eric may have spent a few hours reviewing the code,
maybe drawing diagrams, using check lists, and so on. Why was none of
that documented in the commit message?

The commit message stated the result of your investigation, and none of
the evidence. In order to review the patch, I had to rebuild the entire
argument from zero, checking the life-cycle of every single field in
ACPI_CPU_DATA, and that took the better part of a day.

My job as a reviewer is to *read* your investigation and verify it, not
to *reconstruct* it from scratch.

Documenting one's findings in detail also helps one root out omissions
or mistakes in one's reasoning. I catch my own errors like this all the
time.

Thanks
Laszlo