[Mesa-dev] [v3 PATCH 01/10] mesa:Define extension ARB_framebuffer_no_attachments

[Connor Abbott]

On Thu, May 21, 2015 at 8:40 PM, Rogovin, Kevin <kevin.rogovin at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/mesa-dev