Unified clang format

[ldrumm]

Until now we've had multiple styles throughout the project. This doesn't
buy us anything for contributors as each source file might be different - totally negating the point of having a clang-format style in the first
place - indeed I started debugging clang-format since it was apparently
not formatting my adapter code as 4 spaces after my editor had loaded
the editconfig file specifying 4 spaces. I wasted an hour on that!

Here we codify everything as LLVM style, which seems to be used by the
majority of the project, and change the placement of autogenerated doxygen comments.

n.b. if you want to see what's really happening just look at the first two commits, f68e8c0, and fd6b63b - the others are just regeneration or formatting

intel/llvm#16672

[pbalcer]

This was intentional to keep headers in a more readable format. Also, disabling ReflowComments was necessary to keep the output consistent, as the option is not indempotent.

[ldrumm]

This was intentional to keep headers in a more readable format.

Define "readable".

[pbalcer]

This was intentional to keep headers in a more readable format.

Define "readable".

If I recall correctly, the primary issue was line breaks around comments and enum value definitions. Take a look at this enum: https://github.com/oneapi-src/unified-runtime/blob/main/include/ur_api.h#L415

[ldrumm]

This was intentional to keep headers in a more readable format.

Define "readable".

If I recall correctly, the primary issue was line breaks around comments and enum value definitions. Take a look at this enum: https://github.com/oneapi-src/unified-runtime/blob/main/include/ur_api.h#L415

That problem could be neatly solved by putting the comment on the line before and using /// instead of <///

typedef enum ur_result_t {
  /// Success
  UR_RESULT_SUCCESS = 0,
  /// Invalid operation
  UR_RESULT_ERROR_INVALID_OPERATION = 1,
  /// Invalid queue properties
  UR_RESULT_ERROR_INVALID_QUEUE_PROPERTIES = 2,
  /// Invalid queue
  UR_RESULT_ERROR_INVALID_QUEUE = 3,
  /// Invalid Value
  UR_RESULT_ERROR_INVALID_VALUE = 4,
  /// Invalid context
  UR_RESULT_ERROR_INVALID_CONTEXT = 5,
  /// Invalid platform
  UR_RESULT_ERROR_INVALID_PLATFORM = 6,
  /// Invalid binary
  UR_RESULT_ERROR_INVALID_BINARY = 7,
  /// Invalid program
  UR_RESULT_ERROR_INVALID_PROGRAM = 8,
  /// Invalid sampler
  UR_RESULT_ERROR_INVALID_SAMPLER = 9,
  /// Invalid buffer size
  UR_RESULT_ERROR_INVALID_BUFFER_SIZE = 10,
  /// Invalid memory object
  UR_RESULT_ERROR_INVALID_MEM_OBJECT = 11,
  /// Invalid event
  UR_RESULT_ERROR_INVALID_EVENT = 12,

[ldrumm]

I just checked my above suggestion. This might use more or less vertical space than the existing version, since lines aren't off the right hand side. As an example:

urLoaderConfigGetInfo(
    ur_loader_config_handle_t hLoaderConfig, ///< [in] handle of the loader config object
    ur_loader_config_info_t propName,        ///< [in] type of the info to retrieve
    size_t propSize,                         ///< [in] the number of bytes pointed to by pPropValue.
    void *pPropValue,                        ///< [out][optional][typename(propName, propSize)] array of bytes holding
                                             ///< the info.
                                             ///< If propSize is not equal to or greater than the real number of bytes
                                             ///< needed to return the info
                                             ///< then the ::UR_RESULT_ERROR_INVALID_SIZE error is returned and
                                             ///< pPropValue is not used.
    size_t *pPropSizeRet                     ///< [out][optional] pointer to the actual size in bytes of the queried propName.
);

->

urLoaderConfigGetInfo(
  /// [in] handle of the loader config object
  ur_loader_config_handle_t hLoaderConfig,
  /// [in] type of the info to retrieve
  ur_loader_config_info_t propName,
  /// [in] the number of bytes pointed to by pPropValue.
  size_t propSize,
  /// [out][optional][typename(propName, propSize)] array of bytes holding the
  /// info. If propSize is not equal to or greater than the real number of bytes
  /// needed to return the info then the ::UR_RESULT_ERROR_INVALID_SIZE error is
  /// returned and pPropValue is not used.
  void *pPropValue,
  /// [out][optional] pointer to the actual size in bytes of the queried propName.
  size_t *pPropSizeRet
);

The comment and declaration for pPropValue takes fewer lines here, but the shorter comments occupy more space.

Here is another example, where certain sequences can end up the same length, but with a slightly more consistent appearance : enumerations where the enumerator itself is long, and explicit values for the enumerator declaration:

    UR_FUNCTION_CONTEXT_SET_EXTENDED_DELETER =
      7,                      ///< Enumerator for ::urContextSetExtendedDeleter

->

  /// Enumerator for ::urContextSetExtendedDeleter
  UR_FUNCTION_CONTEXT_SET_EXTENDED_DELETER = 7,

Generalising to all autogenerated comment lines

diff --git a/scripts/templates/helper.py b/scripts/templates/helper.py
index 3b8e5693..678159ef 100644
--- a/scripts/templates/helper.py
+++ b/scripts/templates/helper.py
@@ -768,8 +768,8 @@ def make_etor_lines(namespace, tags, obj, meta=None):
             prologue = "%s,"%(name)
 
         for line in split_line(subt(namespace, tags, item['desc'], True), 70):
-            lines.append("%s%s %s"%(append_ws(prologue, 48), "///<", line))
-            prologue = ""
+            lines.append(" /// %s" % line)
+        lines.append(prologue)
 
     lines += [
         "/// @cond",
@@ -820,8 +820,8 @@ def make_member_lines(namespace, tags, obj, prefix="", meta=None):
         prologue = "%s %s %s;"%(tname, name, array_suffix)
 
         for line in split_line(subt(namespace, tags, item['desc'], True), 70):
-            lines.append("%s%s %s"%(append_ws(prologue, 48), "///<", line))
-            prologue = ""
+            lines.append(" /// %s" % line)
+        lines.append(prologue)
     return lines
 
 """
@@ -869,8 +869,8 @@ def make_param_lines(namespace, tags, obj, decl=False, meta=None, format=["type"
         if "desc" in format:
             desc = item['desc']
             for line in split_line(subt(namespace, tags, desc, True), 70):
-                lines.append("%s///< %s"%(append_ws(prologue, 48), line))
-                prologue = ""
+                lines.append(" /// %s" % line)
+            lines.append(prologue)
         else:
             lines.append(prologue)
 

running the generate target leaves the following files modified:

   include/ur_api.h
   include/ur_ddi.h
   scripts/templates/helper.py
   source/adapters/null/ur_nullddi.cpp
   source/loader/layers/tracing/ur_trcddi.cpp
   source/loader/layers/validation/ur_valddi.cpp
   source/loader/ur_ldrddi.cpp
   source/loader/ur_libapi.cpp
   source/ur_api.cpp

The total line length change for those files:
before:

  12411 include/ur_api.h
   1832 include/ur_ddi.h
   1564 scripts/templates/helper.py
   6818 source/adapters/null/ur_nullddi.cpp
   7650 source/loader/layers/tracing/ur_trcddi.cpp
  10658 source/loader/layers/validation/ur_valddi.cpp
   9173 source/loader/ur_ldrddi.cpp
   8706 source/loader/ur_libapi.cpp
   7390 source/ur_api.cpp
  66202 total

After:

  13051 include/ur_api.h
   1832 include/ur_ddi.h
   1564 scripts/templates/helper.py
   6986 source/adapters/null/ur_nullddi.cpp
   7671 source/loader/layers/tracing/ur_trcddi.cpp
  10153 source/loader/layers/validation/ur_valddi.cpp
   8986 source/loader/ur_ldrddi.cpp
   8770 source/loader/ur_libapi.cpp
   7637 source/ur_api.cpp
  66650 total

That's a less than 1% line length increase for generated code, so I think this could be a win too

[jchlanda]

Here we codify everything as LLVM style, which seems to be used by the majority of the project.

I might be biased, but given the current use of unified runtime - oneAPI, which in turn is forked from llvm, the fact that we bring this repo's style closer to the mainline can only be seen as a net positive.

[MartinWehking]

I agree with @ldrumm and @jchlanda.
It would definitely be better to not have 2 different styles for unified runtime and intel/llvm.

[nrspruit]

LGTM for Level Zero

[ldrumm]

Thanks for the reviews everyone. I've now updated the patch with the suggestions from this comment

The substantive changes are ldrumm@65322c4 and ldrumm@c2610b2. The third commit, is simply the result of running the generate target, and then committing all changes so is entirely mechanical.

It's quite a lot of churn, but the separate commits should make it easy for people to resolve merge conflicts

[ldrumm]

The SPDX license identifier needs to be on its own line according to the spec: https://spdx.github.io/spdx-spec/v2.3/using-SPDX-short-identifiers-in-source-files/

python needs 4 space indents in the .editorconfig

[ldrumm]

The SPDX license identifier needs to be on its own line according to the spec: https://spdx.github.io/spdx-spec/v2.3/using-SPDX-short-identifiers-in-source-files/

python needs 4 space indents in the .editorconfig

Fixen

[ldrumm]

I think this should be good to go if tests pass. I'll add the ready-to-merge label and then wait for something terrible to happen

[kbenzie]

Since this is changing a lot of files all at once I do wonder if this would also be an opportune time to update the clang-format version used from 15 to the version built as part of intel/llvm. This idea is with an eye to existing within the intel/llvm repo moving forwards.

[ldrumm]

Since this is changing a lot of files all at once I do wonder if this would also be an opportune time to update the clang-format version used from 15 to the version built as part of intel/llvm. This idea is with an eye to existing within the intel/llvm repo moving forwards.

Can do. It's trivial for me to remake this patch with new versions of tools / rebase it.
Will you update the clang-format version and I rebase on top of your work or should I add it to this patch series?

[ldrumm]

On Thu Jan 16, 2025 at 2:15 PM GMT, Kenneth Benzie (Benie) wrote: > You happy about https://github.com/cheshirekow/cmake_format for the cmake? If we do switch to that I think it should be separate from this PR, its already very large.

Makes sense. I'll include the editorconfig changes you provided and we can reformat the cmake later As for clang-format, it looks like [dpc++ uses clang-format-18](https://github.com/intel/llvm/blob/f0d0f7cdbcad238bc7492c1977019777defb8996/.github/workflows/pr-code-format.yml#L63). I'll bump the required version in the cmake and requirements.txt

[kbenzie]

Will aim to merge this next once the conflicts are resolved.