> > My only point was that separate concerns should be implemented in > separate patches, or at least (if they are really difficult, or > overkill, to isolate) that they should be documented. > > Please try to think with your reviewers' mindsets in mind, when > preparing a patch (commit message and code both). The question the patch > author has to ask themselves is not only "how do I implement this", but > also "how do I explain this to my reviewers". > > I read the subject line and the commit message. Those make me anticipate > some magic constant (related to 48) in the code. But that's not what I > see in the code. I see new macros, new control flow, new variables, new > indentation. The actual purpose of the patch (as documented in the > commit message) is just a tiny fraction of the whole code change, and > the commit message does not prepare the reader for it. *That* is what's > wrong. Improving code wherever you go is great, but all that effort > needs to be structured correctly, or at least justified in natural language. > > Patches exist primarily for humans to read, and secondarily for > computers to execute. If we don't believe in that, then edk2 will never > become a true open source, community project. (In my opinion anyway.) >
I admit my patch assumes the reviewers should be very familiar to how CPUID "protocol" works. In fact, there are two kinds of reviewers at least: 1. domain reviewers 2. consumers as reviewers I didn't consider the second kind of reviewers. -=-=-=-=-=-=-=-=-=-=-=- Groups.io Links: You receive all messages sent to this group. View/Reply Online (#75410): https://edk2.groups.io/g/devel/message/75410 Mute This Topic: https://groups.io/mt/82765279/21656 Group Owner: devel+ow...@edk2.groups.io Unsubscribe: https://edk2.groups.io/g/devel/unsub [arch...@mail-archive.com] -=-=-=-=-=-=-=-=-=-=-=-