oneapi-src
Unified clang format
Until now we've had multiple styles throught 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.
n.b. if you want to see what's really happening just look at the first commit, 0f218b00fe4e22 - the other commit is just a reformat
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.
This was intentional to keep headers in a more readable format.
Define "readable".
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
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,
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
I agree with @ldrumm and @jchlanda. It would definitely be better to not have 2 different styles for unified runtime and intel/llvm.
Thanks for the reviews everyone. I've now updated the patch with the suggestions from this comment
The substantive changes are https://github.com/ldrumm/unified-runtime/commit/65322c47b7a2c3c3fbbea525ae1385543aa3763a and https://github.com/ldrumm/unified-runtime/commit/c2610b26b22b4af87926319b81eec98f381ff1c3. 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
@pbalcer Are your original concerns assuaged with the latest update?
ping @pbalcer
@pbalcer Are your original concerns assuaged with the latest update?
ping @pbalcer
ping @pbalcer
@pbalcer Are your original concerns assuaged with the latest update?
ping @pbalcer
ping @pbalcer
Apologies, I missed all your previous pings :-)
Yes, I have no objections after the changes. Thanks.
Sorry for letting this rot. I've just rebased it and we can hopefully try again.
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
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
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
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.
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?
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. I'll bump the required version in the cmake and requirements.txt