Hi, As I'm the person who usually generates a lot of "style related" comments I want to reply as well.
Since GitHub does not have a "severity" option for the option it is not too convenient to distinguish between "optional" with "must have" comments. So usually I use the "Comment" button instead of "Request changes" on GitHub. With this I do not want to generate any delay in merging of the changes by other reviewers if they consider my comments as "optional". With my comments I just want to highlight the places that the author needs to pay attention to and either rework that place of reply that this is an intentional change and he/she insists on existing implementation or will app in the next PR or any other type of reply. That is usually a result of my experience as a reviewer because I do not usually have an option to checkout the changes on a laptop and review those with a full scope, so when I suggest to change "if (size)" to "if (size > 0)" that just I want to make sure that author meant it intentionally. because sometimes it really happens that "size" is a pointer and "if (size)" is a typo instead of "if (*size)". I do not claim to be a single source of truth, but rather a "friend giving advice". That is why the review process exists I think. The other reason why I'm writing all these comments is because sometimes people just want to make things faster and just copy/paste code even without looking into it, so my comment is a highlight for the author to rethink the changes and give a meaningful reply or more often to rework the code. I do not want to offend anyone with my comments and I hope that I always write those with respect to the author. What I read in "between the lines" Jukka is that maybe you want to say that authors do not always have the capacity to handle many comments? I as a reviewer do not know the availability of the author and rely only on a reply in GitHub comments. I'm fully fine to merge the changes without many changes that I propose. And sometimes I iterate through the project by myself and fix dozens of style issues that I find and I hope that if each new file added will have less of those that I will not spend too much time later :) Best regards, Petro пт, 25 бер. 2022 р. о 10:32 Xiang Xiao <xiaoxiang781...@gmail.com> пише: > On Fri, Mar 25, 2022 at 3:35 PM Jukka Laitinen <jukka.laiti...@iki.fi> > wrote: > > > Hi, > > > > > > As an another example, we would very much like to bring in > > CONFIG_BUILD_KERNEL support for RISC-V for NuttX, as we have worked hard > > on this for some time, and have it working. Now, even when this work it > > is only additions, not breaking anything for FLAT_BUILD users, > > > Yes, it doesn't break FLAT_BUILD, but doesn't mean KERNEL design or code is > perfect, that's why the interesting people can give the comment. > > > > is stuck > > in endless debate where half of the comments show that the reviewer > > doesn't know the RISC-V ISA thoroughly, > > > Sorry, I give you this impression, but I have worked on RiSCV since 2018:(. > > > > and is referring to how things > > are done in ARM world. And the other half is largely useless style > > nitpicking. > > > > I think the most debate is about how S-mode talks with M-mode in this PR. > The module design and standard compliant is always NuttX core value and > highlight in: > https://github.com/apache/incubator-nuttx/blob/master/INVIOLABLES.md > Since OpenSBI is adopted by many OS to interact with M-mode firmware: > https://github.com/riscv-software-src/opensbi > It's always good to follow the best practice and what I insist in this PR: > we can implement a minimal subset of OpenSBI to support S-mode NuttX, but > don't invent a private interface since M-mode firmware mayn't run NuttX at > all. >
