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

[Ian Romanick]

On 04/18/2016 07:34 PM, Jason Ekstrand wrote:
> 
> 
> On Mon, Apr 18, 2016 at 5:35 PM, Ian Romanick <idr at freedesktop.org
> <mailto: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.
> 
> It's not that bad with block-edit in vim.  Sometimes it's nicer,
> sometimes it's not.
>  
>     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.
> 
> Mind saying what "this" is?

Having the aligned = as the style.

>     Also... it seems like patch 18 should update this. :)

"This" here is the "Eventually, we would like to move to a block-based
packing mechanism."  After patch 18, that is reality, not a possible
future.  Right?

>     > +
>     > +With this new block mechansim, you may end up mixing code and
>     declarations.
>     > +In this case, it's up to the discression of the programmer
>     exactly how to
>     > +tab things but trying to keep with the above rules is recommended.
>     > +
>     > +In meta code, we use a fair number of compound initializers for
>     more easily
>     > +calling Vulkan functions that require struct arguments.  These
>     should be
>     > +declared as follows:
>     > +
>     > +anv_CreateFramebuffer(anv_device_to_handle(device),
>     > +   &(VkFramebufferCreateInfo) {
>     > +      .sType = VK_STRUCTURE_TYPE_FRAMEBUFFER_CREATE_INFO,
>     > +      .attachmentCount = 1,
>     > +      .pAttachments = (VkImageView[]) {
>     > +         anv_image_view_to_handle(dest_iview),
>     > +      },
>     > +      .width = dest_iview->extent.width,
>     > +      .height = dest_iview->extent.height,
>     > +      .layers = 1
>     > +   }, &cmd_buffer->pool->alloc, &fb);
>     > +
>     > +The initial arguments go on the same line as the call and the primary
>     > +struct being passed in is declared on its own line tabbed over 1
>     tab with
>     > +it's contents on the following lines tabbed over an additional tab.
>     > +Substructures get tabbed over by additional tabs as needed.
>     >
> 
>