[clang-format][NFC] Fix a bug in getPreviousToken() in the parser

[curdeius]

Wasn't assert enough? Could you add a test with code that provokes the problem?

[owenpan]

In D116318#3211258, @curdeius wrote:

Wasn't assert enough? Could you add a test with code that provokes the problem?

This fix is an NFC, so there will be no (user) test case for it. :)

The current behavior does not match the description on line 35, and I need this function to be able to return null to simply my code. Otherwise, l would have to do something like the following:

if (Position == 0)
  return false;
const FormatToken *Previous = AllTokens[Position - 1];
assert(Previous);
return Previous->is(tok::comment);

[HazardyKnusperkeks]

I'm against that patch.

Don't pay for what you don't use.

We should not put the size check into every call.

[owenpan]

In D116318#3211689, @HazardyKnusperkeks wrote:

I'm against that patch.

Don't pay for what you don't use.

We should not put the size check into every call.

That's what you must do now without this patch, which will fix the problem. I should be able to just call getPreviousToken() without having to first check Position > 0.

Edit:
I see what you mean now. The question is if getPreviousToken() can be called when the current token is the very first token. I think it can, so you either pay now or later for what you may use.

[curdeius]

I'm in the same position as @HazardyKnusperkeks.
If you need something to simplify the code you can add a helper getPreviousTokenOrNull or something like that in your patch.
But we certainly don't want to pay for the if check each time we call getPreviousToken.

[curdeius]

On another note, you can fix the comment typo without a review.

[owenpan]

In D116318#3212351, @curdeius wrote:

I'm in the same position as @HazardyKnusperkeks.
If you need something to simplify the code you can add a helper getPreviousTokenOrNull or something like that in your patch.
But we certainly don't want to pay for the if check each time we call getPreviousToken.

I think we are going overboard here. Based on the comments, the intent was to return null for the previous token of the first token. I was the first and only one who needed to call it, and if the caller must check if it's the first token every time anyway, why not just do it in the callee and make the function more robust. Perhaps we can rename the current one to something like getPreviousTokenFast?

[MyDeveloperDay]

I agree performance of this if is unlikely to be a game changer performance wise

clang/lib/Format/UnwrappedLineParser.cpp

 
 class FormatTokenSource {
 public:
   virtual ~FormatTokenSource() {}
 
   // Returns the next token in the token stream.
   virtual FormatToken *getNextToken() = 0;
 
-  // Returns the token precedint the token returned by the last call to
+  // Returns the token preceding the token returned by the last call to
   // getNextToken() in the token stream, or nullptr if no such token exists.
   virtual FormatToken *getPreviousToken() = 0;
 
   // Returns the token that would be returned by the next call to
   // getNextToken().
   virtual FormatToken *peekNextToken() = 0;
 
   // Returns whether we are at the end of the file.
FormatToken *getNextToken() override {
     LLVM_DEBUG({
       llvm::dbgs() << "Next ";
       dbgToken(Position);
     });
     return Tokens[Position];
   }
 
   FormatToken *getPreviousToken() override {
-    assert(Position > 0);
-    return Tokens[Position - 1];
+    return Position > 0 ? Tokens[Position - 1] : nullptr;
   }
 
   FormatToken *peekNextToken() override {
     int Next = Position + 1;
     LLVM_DEBUG({
       llvm::dbgs() << "Peeking ";
       dbgToken(Next);
     });