[Mesa-dev] [PATCH 01/18] anv: Add a short style guide

[Ian Romanick]

On 04/18/2016 05:56 PM, Dave Airlie wrote:
> On 19 April 2016 at 10:35, Ian Romanick <idr at freedesktop.org> wrote:
>> On 04/18/2016 05:10 PM, Jason Ekstrand wrote:
>>> ---
>>>  src/intel/vulkan/STYLE | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++
>>>  1 file changed, 67 insertions(+)
>>>  create mode 100644 src/intel/vulkan/STYLE
>>>
>>> diff --git a/src/intel/vulkan/STYLE b/src/intel/vulkan/STYLE
>>> new file mode 100644
>>> index 0000000..4eb8f79
>>> --- /dev/null
>>> +++ b/src/intel/vulkan/STYLE
>>> @@ -0,0 +1,67 @@
>>> +The Intel Vulkan driver typically follows the mesa coding style with a few
>>> +exceptions.  First is that structs declared in anv_private.h should be
>>> +written as follows:
>>> +
>>> +struct anv_foo {
>>> +   int                  short_type;
>>> +   struct anv_long_type long_type;
>>> +   void *               ptr;
>>> +};
>>> +
>>> +Where the * for pointers goes one space after the type and the names are
>>> +vertically aligned.  The names should be tabbed over the minimum amount
>>> +(still a multiple of 3 spaces) such that they can all be aligned.
>>> +
>>> +When the anv_batch_emit function is used, it should look as follows:
>>> +
>>> +anv_batch_emit(&cmd_buffer->batch, GENX(3DPRIMITIVE),
>>> +   .VertexAccessType       = SEQUENTIAL,
>>> +   .PrimitiveTopologyType  = pipeline->topology,
>>> +   .VertexCountPerInstance = vertexCount,
>>> +   .StartVertexLocation    = firstVertex,
>>> +   .InstanceCount          = instanceCount,
>>> +   .StartInstanceLocation  = firstInstance,
>>> +   .BaseVertexLocation     = 0);
>>> +
>>> +The batch and struct name parameters should go on the same line with the
>>> +anv_batch_emit call and each named parameter on its own line.  The
>>> +alignment rules are the same as for structs where all of the "=" are
>>> +vertically aligned at the minimum tabstop required to do so.
>>> +
>>> +Eventually, we would like to move to a block-based packing mechanism.  In
>>> +this case, the above packing macro would look like this:
>>> +
>>> +anv_batch_emit(&cmd_buffer->batch, GENX(3DPRIMITIVE), prim) {
>>> +   prim.VertexAccessType         = SEQUENTIAL;
>>> +   prim.PrimitiveTopologyType    = pipeline->topology;
>>> +   prim.VertexCountPerInstance   = vertexCount;
>>> +   prim.StartVertexLocation      = firstVertex;
>>> +   prim.InstanceCount            = instanceCount;
>>> +   prim.StartInstanceLocation    = firstInstance;
>>> +   prim.BaseVertexLocation       = 0;
>>> +}
>>
>> When I first read the style guide, I thought, "Man, that looks nice."
>> Then I read all the patches, and I thought, "Man, that looks irritating
>> to type."  I don't know a way to make any text editor (or indent) do
>> this for me automatically, so it seems really irritating to write.
>>
>> Are we sure we want to commit to doing this?  It would be easy enough to
>> change now using sed on the patches, but changing it later will be much
>> more painful.
>>
>> Also... it seems like patch 18 should update this. :)
> 
> Really we should just pick kernel style or Mesa style. I'm not sure we
> need to invest

Well... I tend to agree, but we're doing things here that we haven't
previously done in Mesa, and I don't know that the kernel does (much?)
of this either.

> in a new style. If your style guide isn't an indent command line and
> .emacs and .vim
> snippets, then it isn't near as useful. Also as I said on irc,

It definitely decreases the probability that people will get it
consistently right... and increases the number of vN+1 patches.

> separating the * from the name
> is bad documentation practice.
> 
> I can spot the bug in both lines below a lot easier in the second. The
> * goes with the name
> not with the type.
> 
> uint32_t *              ptr1, ptr2;
> uint32_t                *ptr1, ptr2;

Right.  That's part of the reason Mesa doesn't generally declare
multiple variables in a single line like that.

I have seen similar styles that do

  int               foo;
  int              *bar;
  int               asdf;

The names still line up, the but the * is someplace sensible.

> Dave.