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



_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to