[PATCH] D103678: [Format] Fix incorrect pointer/reference detection

[Yilong Guo via Phabricator via cfe-commits]

Nuullll added inline comments.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:236
+        Prev = Prev->Previous;
+        assert(Prev);
+      }
----------------
MyDeveloperDay wrote:
> curdeius wrote:
> > MyDeveloperDay wrote:
> > > Do we need to worry about `Prev` ever being null? Does the assert ever fire? if not why have it in the first place?
> > > 
> > > We'll crash if it is, do we want to guard against that?
> > Disclaimer: I'm in the pro-assert camp :).
> > In general, I'd preferably keep this assert because people sometimes use Release+Assertions build. Even more those that report bugs.
> > And it's much nicer and easier to fix  an assertion failure that is well defined and precisely localised than a less clearer report of a crash.
> > Of course, in a build without assertions, it will crash if `Prev` is null, but still it will be easier to find the bug when trying to reproduce.
> > 
> > For this specific case, this assertion should never fire because otherwise it would mean that we detected had `Left` being `TT_OverloadedOperatorLParen` but there was no corresponding `tok::kw_operator` (or just `operator`). So it would mean that there's a bug in (probably) token annotator. So yeah, it falls into "should never happen" category about which assertions are all about IMO.
> To be honest I'm in the assert and guard as well camp (if such a camp exists ;-)
> 
> I have this discussion regularly with the other developers in my team, the use assert is useless other than for our assertion builds (which in  high performance code, almost no one runs, until there is a problem!)
> 
> The assert triggers in my mind that it MIGHT be null (but generally shouldn't) so in which case we have to guard as well for the rare case, but I like the assert.. but please of the form:
> 
> ```assert(condition && "Some nice string telling me exactly what went wrong")```
> 
> When the assert fires I get a nice error message, not just assert failed with `0`
> 
> https://llvm.org/docs/CodingStandards.html#assert-liberally
> 
@curdeius @MyDeveloperDay Thanks for the explanations. I've added an error message.


================
Comment at: clang/unittests/Format/FormatTest.cpp:8283-8286
+  // TODO:
+  // Calling an operator as a non-member function is hard to distinguish
+  // verifyFormat("void f() { operator*(a & a); }");
+  // verifyFormat("void f() { operator&(a, b * b); }");
----------------
MyDeveloperDay wrote:
> curdeius wrote:
> > curdeius wrote:
> > > Nuullll wrote:
> > > > This patch doesn't fix these, i.e. when operators are called as non-member functions.
> > > > The call sites seem to be marked as function declarations.
> > > Is there a bug report on that? If so, please add a link to the comment. If no, could you please create one?
> > > Or is it really something that will never be possible to distinguish (hmm, this would be strange)?
> > Nit: full-stop.
> Might be nice just to fix these as well, this is kind of what happens with clang-format development... you fix one thing we try to make you fix everything else around it, otherwise who is going to understand it better than you! ;-)
> 
> We do appreciate your patch!!
@curdeius 

> Is there a bug report on that? If so, please add a link to the comment. If no, could you please create one?

No. I've created one and added the link in the comment!

> Or is it really something that will never be possible to distinguish (hmm, this would be strange)?

I think it should be distinguishable, at least there should be no return type before the operator in the context of non-member function invocation. I'd like to fix this in a separate patch, later. I need some time to figure it out :)


================
Comment at: clang/unittests/Format/FormatTest.cpp:8283-8286
+  // TODO:
+  // Calling an operator as a non-member function is hard to distinguish
+  // verifyFormat("void f() { operator*(a & a); }");
+  // verifyFormat("void f() { operator&(a, b * b); }");
----------------
Nuullll wrote:
> MyDeveloperDay wrote:
> > curdeius wrote:
> > > curdeius wrote:
> > > > Nuullll wrote:
> > > > > This patch doesn't fix these, i.e. when operators are called as non-member functions.
> > > > > The call sites seem to be marked as function declarations.
> > > > Is there a bug report on that? If so, please add a link to the comment. If no, could you please create one?
> > > > Or is it really something that will never be possible to distinguish (hmm, this would be strange)?
> > > Nit: full-stop.
> > Might be nice just to fix these as well, this is kind of what happens with clang-format development... you fix one thing we try to make you fix everything else around it, otherwise who is going to understand it better than you! ;-)
> > 
> > We do appreciate your patch!!
> @curdeius 
> 
> > Is there a bug report on that? If so, please add a link to the comment. If no, could you please create one?
> 
> No. I've created one and added the link in the comment!
> 
> > Or is it really something that will never be possible to distinguish (hmm, this would be strange)?
> 
> I think it should be distinguishable, at least there should be no return type before the operator in the context of non-member function invocation. I'd like to fix this in a separate patch, later. I need some time to figure it out :)
> Might be nice just to fix these as well, this is kind of what happens with clang-format development... you fix one thing we try to make you fix everything else around it, otherwise who is going to understand it better than you! ;-)
> 
> We do appreciate your patch!!

@MyDeveloperDay Thanks for reviewing this. I will try to fix these in another patch. It's always exciting to contribute to llvm!


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

https://reviews.llvm.org/D103678