On Thu, May 21, 2015 at 8:40 PM, Rogovin, Kevin <kevin.rogo...@intel.com> wrote: > > >> I suppose it could be useful, but I think we've been mostly successful at >> just expecting people to recognize when what they're writing doesn't look >> like the code around it. > > This is my point. Older code had different style/expectations than newer > code. For this patch series, I have hit a number of nits and quasi-nits that > are ambiguous: > > 0. In v1 to v2, avoid GL types atleast in i965 although much of the code uses > that (the explanation given was to move away from GL types). Does this apply > to Mesa core as well? > > 1. the casting bit in the i965 patch to use the _mesa_geomety_ functions. > These are not quite nits, but in some ways not a big deal. I hit these > because there were ints there before. In this regard doing what was there > before was ungood (atleast for this review). > > 2. the line between function thing. In truth I just missed that extra line > for the added to framebuffer.h (and I should not have), but there are places > in the code that there are multiple empty lines between function definitions. > I do not mind saying "no extra empty lines", but not knowing the rules and > attempting to infer them from the code, I seem to hit too many nits. > > 3. Even on the subject of git commit, I am seeing different answers: 75, but > try 50 usually, but understandable if cannot do it. Utterly ambiguous.
No, that's because you misread it. The rules for commit messages (and usually, for everything else as well) have a purpose, and that's why they might seem "ambiguous" to you: we're adults here, so we don't need to have a huge debate on 75 vs. 72 chars for commit message bodies, because we know that both satisfy the *purpose* of being able to do 'git log' on an 80-char terminal without ugly line-wrapping due to 'git log' indenting your message an extra 4 spaces. Sometimes we get too wrapped up in sweating the details, but in the end it's just about consistency and making things easier for people. You're also missing the difference between the subject line, which is the thing people will skim over when looking for something and therefore should be as short as possible (ideally under 50 chars), and the body (the part after the empty line), which is what people already looking at your commit will read to fully understand what it's changing. The body can be as long as it needs to be, as long as it's concise and to the point, and it should be wrapped to at most 75 chars. > > 4. on the subject of line length I have this so far: usually 78, but 80 > sometimes is ok. Does ok, for example, include making a large-ish comment > block more justified? > > 5. Consecutive empty lines is not ok, except in function definitions, but > then only sometimes. I am guessing "sometimes" is for grouping function > definitions, but plenty of files follow different conventions (hence what > Brian Paul said). > > Given that nits just add traffic (and I want to minimize that silliness) I > think it would be wise to set down some precise rules so there is no > judgement calls required for something as silly as formatting nits. Ideally, > we would have a lint script that would do it for us :) I see that reviews > usually first hit nits, then content of patches. That is fine, but I'd rather > know all the rules rather than hitting the nits at all. > > Again, I really have no preference since someone is paying me to do this, but > knowing the exact rules would be more efficient. Inferring rules is quite > error prone IMO (indeed, at one point, the GLSL pre-processor appeared to be > written in a different style). > > -Kevin > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
