format: Remove redundant calls to guessIsObjC to speed up clang-format on unknown file types

[davidvc1]

Summary

Running clang-format on the following input

int lambdas() {
  return [&] { 
  return [&] { 
  return [&] { 
  return [&] { 
  return [&] { 
  return [&] { 
  return [&] { return 3; } (); 
  } (); } (); } (); } (); } (); } (); }

will finish instantly if you pass clang-format a .cpp input with this content,
but hang for tens of seconds if you pass the same via stdin.

Adding some debug statements showed that guessIsObjC was getting called
tens of millions of times in a manner that scales very rapidly with the amount
of nesting (if clang-format just takes a few seconds with that input passed
on stdin, try adding a couple more levels of nesting).

This change moves the recursive guessIsObjC call one level of nesting out of
an inner loop whose iterations don't affect the input to the recursive call. This
resolves the performance issue.

[davidvc1]

Hi @curdeius, I'm new to the project and I'm adding you as a reviewer because I noticed you reviewed a number of recent changes in this directory. Are you a good reviewer for this change? Thanks!

[davidvc1]

+ @rsmith from CODE_OWNERS.txt: Hi Richard, could you please suggest a reviewer for this change? Thanks!

[MyDeveloperDay]

This needs a full context diff and ideally some unit tests

[curdeius]

I see indeed that this change fixes the superlinear complexity when formatting code from stdin:

cat test.cpp | time clang-format --style=LLVM

0:01.44 ==   1.44s  //  9 returns
0:13.17 ==  13s     // 10 returns
1:56.63 == 116s     // 11 returns

time clang-format --style=LLVM test.cpp
0.03s-0.05s

And after this change the stdin case takes as much time as with a file.

It's definitely a good change, but I'd like to understand what's the reason, why stdin is handled so much differently?
Have you investigated this?

Also, a sort of performance test (e.g. a test with a timeout) would be nice to have.

[davidvc1]

@curdeius , the PR description mentions the reason: when you pipe input from stdin, clang-format has to run through the "is it objective C?" codepath, and this is the codepath with the bug.

On the other hand, when you pass a .cpp file as a file path input, it doesn't need to figure out whether the code is objective C.

[owenpan]

We can land this as NFC. @davidvc1 what email do you want to use for the authorship?

NOTE: Clang-Format Team Automated Review Comment

It looks like your clang-format review does not contain any unit tests, please try to ensure all code changes have a unit test (unless this is an NFC or refactoring, adding documentation etc..)

Add your unit tests in clang/unittests/Format and you can build with ninja FormatTests. We recommend using the verifyFormat(xxx) format of unit tests rather than EXPECT_EQ as this will ensure you change is tolerant to random whitespace changes (see FormatTest.cpp as an example)

For situations where your change is altering the TokenAnnotator.cpp which can happen if you are trying to improve the annotation phase to ensure we are correctly identifying the type of a token, please add a token annotator test in TokenAnnotatorTest.cpp

clang/lib/Format/Format.cpp

                      << getTokenTypeName(FormatTok->getType()) << "\n");
           return true;
         }
-        if (guessIsObjC(SourceManager, Line->Children, Keywords))
-          return true;
       }
+      if (guessIsObjC(SourceManager, Line->Children, Keywords))
+        return true;
     }
     return false;
   }