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

[MyDeveloperDay via Phabricator via cfe-commits]

MyDeveloperDay added inline comments.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:236
+        Prev = Prev->Previous;
+        assert(Prev);
+      }
----------------
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



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D103678