[PATCH] D158293: [NFC][CLANG] Fix potential dereferencing of null return values

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.


================
Comment at: clang/lib/Format/TokenAnnotator.cpp:2009-2010
                (Line.MightBeFunctionDecl || Line.InPPDirective) &&
-               Current.NestingLevel == 0 &&
+               Current.NestingLevel == 0 && Current.Previous &&
                !Current.Previous->isOneOf(tok::kw_operator, tok::identifier)) {
       // not auto operator->() -> xxx;
----------------
tahonermann wrote:
> owenpan wrote:
> > `Current.Previous` can't be null here because `AutoFound` is `true`.
> Could you please elaborate on why you believe it is safe to move the check of `Current.Previous` inside the body of the `if` statement? Doing so will short circuit the remaining `else if` cases such that `Current.setType()` will not be called at all. It isn't obvious to me that those cases should not be considered if the previous token was not one of `kw_operator` or `identifier`. This looks like it has potential to change behavior.
> 
> The change that was originally proposed is clearly safe.
> Could you please elaborate on why you believe it is safe to move the check of `Current.Previous` inside the body of the `if` statement? Doing so will short circuit the remaining `else if` cases such that `Current.setType()` will not be called at all. It isn't obvious to me that those cases should not be considered if the previous token was not one of `kw_operator` or `identifier`. This looks like it has potential to change behavior.

Ahh, you are right.

> The change that was originally proposed is clearly safe.

My point that `Previous` can't be null still stands. So we should either make no changes here or add an assertion just before the `if` statement at line 1991 above.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158293