[PATCH] D75364: [clang-format] Handle macros in function params and return value

[Tamas Petz via Phabricator via cfe-commits]

tamas.petz marked 2 inline comments as done.
tamas.petz added a comment.

@all, thank you for the review so far.

The case I am trying to handle is way too ambiguous.
IMHO looking at tokens only is not going to lead to a "perfect" formatter.
The case I am trying to handle is quite common but it can lead to incorrect formatting in other cases.

I had a weak hope that passing all tests is "safe enough" but it is unfortunately not.

Should we just note that cases similar to the one in the description are just too ambiguous to handle correctly?
Does anyone see a way the highlighted formatting issue can be solved?

Many thanks,

- Tamas



================
Comment at: clang/lib/Format/TokenAnnotator.cpp:313
+        // for example:
+        //   void f(volatile ElfW(Addr)* addr = nullptr);
+        if (HasStarToken) {
----------------
MyDeveloperDay wrote:
> I assume it could be almost anything?
> 
> void f(volatile ElfW(Addr)& addr);
> void f(volatile ElfW(Addr)&& addr);
> void f(volatile ElfW(Addr) const & addr);
> 
> void f(volatile ElfW(Addr,foo)* addr);
> void f(volatile ElfW(Addr,ElfW(Addr) *foo)* addr);
> 
> ? you seem to handle only the * case
Yes, I am handling this case only at the moment.
I am not sure this patch is going to land at all so I spared some work until we figure it out.


================
Comment at: clang/unittests/Format/FormatTest.cpp:7360
+  verifyFormat("int f(M(x) *p1 = nullptr, M(x) *p2, volatile M(x) *p3);");
+  verifyFormat("M(x) *foo();");
+  verifyFormat("const M(x) *foo(M(x) *a = nullptr);");
----------------
krasimir wrote:
> This is ambiguous: the `*` could be a binary operator: https://godbolt.org/z/n7Jr-h
This case is ambiguous. Like the case at line 7324, which could also be "MACRO()* ptr;"


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D75364/new/

https://reviews.llvm.org/D75364