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

[Dave Airlie]

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
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,
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;

Dave.