[clang][AST] Fix spaces in TypePrinter for some calling convs

[sarnex]

There needs to be a space as the first character, otherwise the printed function prototype will have the CC attribute attached to the final ).

I noticed this looking at the AST for a function with __attribute__((device_kernel))

[llvmbot]

@llvm/pr-subscribers-clang

Author: Nick Sarnie (sarnex)

Changes

There needs to be a space as the first character, otherwise the printed function prototype will have the CC attribute attached to the final ).

I noticed this looking at the AST for a function with __attribute__((device_kernel))

---

Full diff: https://github.com/llvm/llvm-project/pull/143160.diff

1 Files Affected:

• (modified) clang/lib/AST/TypePrinter.cpp (+3-3)

diff --git a/clang/lib/AST/TypePrinter.cpp b/clang/lib/AST/TypePrinter.cpp
index 330cfcd962825..d18723d807c6a 100644
--- a/clang/lib/AST/TypePrinter.cpp
+++ b/clang/lib/AST/TypePrinter.cpp
@@ -1095,13 +1095,13 @@ void TypePrinter::printFunctionAfter(const FunctionType::ExtInfo &Info,
       OS << " __attribute__((pcs(\"aapcs-vfp\")))";
       break;
     case CC_AArch64VectorCall:
-      OS << "__attribute__((aarch64_vector_pcs))";
+      OS << " __attribute__((aarch64_vector_pcs))";
       break;
     case CC_AArch64SVEPCS:
-      OS << "__attribute__((aarch64_sve_pcs))";
+      OS << " __attribute__((aarch64_sve_pcs))";
       break;
     case CC_DeviceKernel:
-      OS << "__attribute__((device_kernel))";
+      OS << " __attribute__((device_kernel))";
       break;
     case CC_IntelOclBicc:
       OS << " __attribute__((intel_ocl_bicc))";

[mizvekov]

Change looks good, but it would be better to include a regression test.

[sarnex]

Thanks, will do.

[sarnex]

Just pushed a commit with a test, thanks!

[mizvekov]

Can you make this test a positive pattern, instead of a CHECK-NOT? Otherwise the test is a bit fragile.
This shouldn't be problem, since you already have strict-whitespace passed to FileCheck.

[mizvekov]

Also, with a positive pattern, it shouldn't be necessary to run the test three separate times, and you can remove the #ifdefs

[sarnex]

Thanks for the review. I did the negative check because before this change the AST print is a little different for some reason where it contains the good string as part of the overall string, ex:

foo3 'void () __attribute__((aarch64_sve_pcs))':'void ()__attribute__((aarch64_sve_pcs))'

and after the fix the output is only

foo3 'void () __attribute__((aarch64_sve_pcs))'

so checking for foo3 'void () __attribute__((aarch64_sve_pcs))' doesn't work because it's a substring in the bad case.

but I managed to fix it by using regex and checking for EOL, it's not super pretty though, just pushed it, thanks.

[mizvekov]

Thanks, LGTM!

You might want to consider adding a change to the ReleaseNotes before merging this.

[mizvekov]

Suggested change

	// CHECK: {{foo1 'void \(\) __attribute__\(\(device_kernel\)\)'$}}
	// CHECK: foo1 'void () __attribute__((device_kernel))'{{$}}

You don't need to apply the regex to the whole match, this makes it a little bit easier to read.

[sarnex]

applied this change and added a release note, thanks so much!