clang-format: Wrong spacing in macro with C-style cast and address-of operator #83400

akien-mga · 2024-02-29T10:03:56Z

Reproducible in 17.0.0 to 17.0.6
Not reproducible in 16.0.6 and earlier
18+ not tested yet, will do when it lands in Fedora 39 or the pre-commit mirror

We noticed a regression in clang-format when moving from clang-format 16 to 17 in godotengine/godot: godotengine/godot#88959

The following test case reproduces the issue with the default clang-format config:

#include <cstdint>
#include <cstdio>

#define PRINT_ADDRESS(m_name) printf("%d\n", (uint64_t)&m_name)

void func1() {}
void func2() {}

int main() {
  PRINT_ADDRESS(func1);
  PRINT_ADDRESS(func2);
  return 0;
}

This is formatted as is by clang-format 16, but running clang-format 17 generates the following diff:

diff --git a/main.cpp b/main.cpp
index c325f7a..81a6329 100644
--- a/main.cpp
+++ b/main.cpp
@@ -1,7 +1,7 @@
 #include <cstdint>
 #include <cstdio>
 
-#define PRINT_ADDRESS(m_name) printf("%d\n", (uint64_t)&m_name)
+#define PRINT_ADDRESS(m_name) printf("%d\n", (uint64_t) & m_name)
 
 void func1() {}
 void func2() {}

It seems to treat & as a bitwise operator instead of address-of operator.

Here's a reproduction project with this test case, and a config for pre-commit to run both clang-format 16 and 17 in turns. You can set it up with:

pip install pre-commit
pre-commit install
pre-commit run -a

clang_format_operator_spacing.zip

Real life examples found in Godot:

diff --git a/core/extension/gdextension_interface.cpp b/core/extension/gdextension_interface.cpp
index 0c96c32187a55..f96a8873cafd9 100644
--- a/core/extension/gdextension_interface.cpp
+++ b/core/extension/gdextension_interface.cpp
@@ -1415,7 +1415,7 @@ static void gdextension_editor_help_load_xml_from_utf8_chars(const char *p_data)
 #endif
 }
 
-#define REGISTER_INTERFACE_FUNC(m_name) GDExtension::register_interface_function(#m_name, (GDExtensionInterfaceFunctionPtr)&gdextension_##m_name)
+#define REGISTER_INTERFACE_FUNC(m_name) GDExtension::register_interface_function(#m_name, (GDExtensionInterfaceFunctionPtr) & gdextension_##m_name)
 
 void gdextension_setup_interface() {
 	REGISTER_INTERFACE_FUNC(get_godot_version);
diff --git a/core/variant/variant_internal.h b/core/variant/variant_internal.h
index 171074188f629..79bed9be33709 100644
--- a/core/variant/variant_internal.h
+++ b/core/variant/variant_internal.h
@@ -810,7 +810,7 @@ struct VariantInternalAccessor<bool> {
 #define VARIANT_ACCESSOR_NUMBER(m_type)                                                                        \
 	template <>                                                                                                \
 	struct VariantInternalAccessor<m_type> {                                                                   \
-		static _FORCE_INLINE_ m_type get(const Variant *v) { return (m_type)*VariantInternal::get_int(v); }    \
+		static _FORCE_INLINE_ m_type get(const Variant *v) { return (m_type) * VariantInternal::get_int(v); }  \
 		static _FORCE_INLINE_ void set(Variant *v, m_type p_value) { *VariantInternal::get_int(v) = p_value; } \
 	};
 
diff --git a/core/variant/variant_setget.cpp b/core/variant/variant_setget.cpp
index 50c9c10987b2f..20941b944fb3f 100644
--- a/core/variant/variant_setget.cpp
+++ b/core/variant/variant_setget.cpp
@@ -433,9 +433,9 @@ Variant Variant::get_named(const StringName &p_member, bool &r_valid) const {
 			}                                                                                                                        \
 			m_assign_type num;                                                                                                       \
 			if (value->get_type() == Variant::INT) {                                                                                 \
-				num = (m_assign_type)*VariantGetInternalPtr<int64_t>::get_ptr(value);                                                \
+				num = (m_assign_type) * VariantGetInternalPtr<int64_t>::get_ptr(value);                                              \
 			} else if (value->get_type() == Variant::FLOAT) {                                                                        \
-				num = (m_assign_type)*VariantGetInternalPtr<double>::get_ptr(value);                                                 \
+				num = (m_assign_type) * VariantGetInternalPtr<double>::get_ptr(value);                                               \
 			} else {                                                                                                                 \
 				*oob = false;                                                                                                        \
 				*valid = false;                                                                                                      \
@@ -495,9 +495,9 @@ Variant Variant::get_named(const StringName &p_member, bool &r_valid) const {
 			}                                                                                                                  \
 			m_assign_type num;                                                                                                 \
 			if (value->get_type() == Variant::INT) {                                                                           \
-				num = (m_assign_type)*VariantGetInternalPtr<int64_t>::get_ptr(value);                                          \
+				num = (m_assign_type) * VariantGetInternalPtr<int64_t>::get_ptr(value);                                        \
 			} else if (value->get_type() == Variant::FLOAT) {                                                                  \
-				num = (m_assign_type)*VariantGetInternalPtr<double>::get_ptr(value);                                           \
+				num = (m_assign_type) * VariantGetInternalPtr<double>::get_ptr(value);                                         \
 			} else {                                                                                                           \
 				*oob = false;                                                                                                  \
 				*valid = false;                                                                                                \

But you can also see in the same PR godotengine/godot#88959 that other cases where & is a bitwise operator were fixed by clang-format. I'm not sure there's any easy way for clang-format to know which is which, if so I understand if this is considered a "won't fix".

The text was updated successfully, but these errors were encountered:

rymiel · 2024-02-29T11:18:52Z

This change was made in 7a4cdbe (https://reviews.llvm.org/D153745)

Fixes llvm#83400.

owenca · 2024-03-09T03:41:24Z

Real life examples found in Godot:

diff --git a/core/extension/gdextension_interface.cpp b/core/extension/gdextension_interface.cpp
index 0c96c32187a55..f96a8873cafd9 100644
--- a/core/extension/gdextension_interface.cpp
+++ b/core/extension/gdextension_interface.cpp
@@ -1415,7 +1415,7 @@ static void gdextension_editor_help_load_xml_from_utf8_chars(const char *p_data)
 #endif
 }
 
-#define REGISTER_INTERFACE_FUNC(m_name) GDExtension::register_interface_function(#m_name, (GDExtensionInterfaceFunctionPtr)&gdextension_##m_name)
+#define REGISTER_INTERFACE_FUNC(m_name) GDExtension::register_interface_function(#m_name, (GDExtensionInterfaceFunctionPtr) & gdextension_##m_name)
 
 void gdextension_setup_interface() {
 	REGISTER_INTERFACE_FUNC(get_godot_version);
diff --git a/core/variant/variant_internal.h b/core/variant/variant_internal.h
index 171074188f629..79bed9be33709 100644
--- a/core/variant/variant_internal.h
+++ b/core/variant/variant_internal.h
@@ -810,7 +810,7 @@ struct VariantInternalAccessor<bool> {
 #define VARIANT_ACCESSOR_NUMBER(m_type)                                                                        \
 	template <>                                                                                                \
 	struct VariantInternalAccessor<m_type> {                                                                   \
-		static _FORCE_INLINE_ m_type get(const Variant *v) { return (m_type)*VariantInternal::get_int(v); }    \
+		static _FORCE_INLINE_ m_type get(const Variant *v) { return (m_type) * VariantInternal::get_int(v); }  \
 		static _FORCE_INLINE_ void set(Variant *v, m_type p_value) { *VariantInternal::get_int(v) = p_value; } \
 	};
 
diff --git a/core/variant/variant_setget.cpp b/core/variant/variant_setget.cpp
index 50c9c10987b2f..20941b944fb3f 100644
--- a/core/variant/variant_setget.cpp
+++ b/core/variant/variant_setget.cpp
@@ -433,9 +433,9 @@ Variant Variant::get_named(const StringName &p_member, bool &r_valid) const {
 			}                                                                                                                        \
 			m_assign_type num;                                                                                                       \
 			if (value->get_type() == Variant::INT) {                                                                                 \
-				num = (m_assign_type)*VariantGetInternalPtr<int64_t>::get_ptr(value);                                                \
+				num = (m_assign_type) * VariantGetInternalPtr<int64_t>::get_ptr(value);                                              \
 			} else if (value->get_type() == Variant::FLOAT) {                                                                        \
-				num = (m_assign_type)*VariantGetInternalPtr<double>::get_ptr(value);                                                 \
+				num = (m_assign_type) * VariantGetInternalPtr<double>::get_ptr(value);                                               \
 			} else {                                                                                                                 \
 				*oob = false;                                                                                                        \
 				*valid = false;                                                                                                      \
@@ -495,9 +495,9 @@ Variant Variant::get_named(const StringName &p_member, bool &r_valid) const {
 			}                                                                                                                  \
 			m_assign_type num;                                                                                                 \
 			if (value->get_type() == Variant::INT) {                                                                           \
-				num = (m_assign_type)*VariantGetInternalPtr<int64_t>::get_ptr(value);                                          \
+				num = (m_assign_type) * VariantGetInternalPtr<int64_t>::get_ptr(value);                                        \
 			} else if (value->get_type() == Variant::FLOAT) {                                                                  \
-				num = (m_assign_type)*VariantGetInternalPtr<double>::get_ptr(value);                                           \
+				num = (m_assign_type) * VariantGetInternalPtr<double>::get_ptr(value);                                         \
 			} else {                                                                                                           \
 				*oob = false;                                                                                                  \
 				*valid = false;                                                                                                \

But you can also see in the same PR godotengine/godot#88959 that other cases where & is a bitwise operator were fixed by clang-format. I'm not sure there's any easy way for clang-format to know which is which, if so I understand if this is considered a "won't fix".

C-style casts to user-defined types can be handled by the TypeNames option. There's also the SkipMacroDefinitionBody option that tells clang-format to skip macro definitions.

Fixes #83400.

arichardson · 2024-03-09T04:41:43Z

While I agree that the patch linked below is a good improvement to the clang-format heuristics,
I don't think it really helps fixing the underlying issue.

I agree with @zygoloid's comments in the linked PR that (identifier)*expr/(identifier)&expr is going to almost always be a C-style cast unless we are inside a macro definition.
And if we add the heuristic that only parenthesized macro arguments should not be treated as types it also fixes the REGISTER_INTERFACE_FUNC example above.

owenca · 2024-03-09T06:04:28Z

@arichardson patches are welcome!

akien-mga mentioned this issue Feb 29, 2024

Pre-commit: Update to clang-format 17.0.6 and black 24.2.0 godotengine/godot#88959

Merged

github-actions bot added the clang-format label Feb 29, 2024

owenca self-assigned this Mar 1, 2024

owenca added a commit to owenca/llvm-project that referenced this issue Mar 3, 2024

[clang-format] Handle common C++ non-keyword types as such

91d6e4c

Fixes llvm#83400.

owenca mentioned this issue Mar 3, 2024

[clang-format] Handle common C++ non-keyword types as such #83709

Merged

owenca closed this as completed in #83709 Mar 9, 2024

owenca added a commit that referenced this issue Mar 9, 2024

[clang-format] Handle common C++ non-keyword types as such (#83709)

0baef3b

Fixes #83400.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clang-format: Wrong spacing in macro with C-style cast and address-of operator #83400

clang-format: Wrong spacing in macro with C-style cast and address-of operator #83400

akien-mga commented Feb 29, 2024 •

edited

Loading

rymiel commented Feb 29, 2024

owenca commented Mar 9, 2024

arichardson commented Mar 9, 2024

owenca commented Mar 9, 2024

clang-format: Wrong spacing in macro with C-style cast and address-of operator #83400

clang-format: Wrong spacing in macro with C-style cast and address-of operator #83400

Comments

akien-mga commented Feb 29, 2024 • edited Loading

rymiel commented Feb 29, 2024

owenca commented Mar 9, 2024

arichardson commented Mar 9, 2024

owenca commented Mar 9, 2024

akien-mga commented Feb 29, 2024 •

edited

Loading