[PATCH] Don't remove unknown tokens that are not whitespace (PR17215).

[Daniel Jasper]

================
Comment at: lib/Format/TokenAnnotator.cpp:573
@@ -571,1 +572,3 @@
   void determineTokenType(FormatToken &Current) {
+    if (Current.Previous && Current.Previous->Tok.is(tok::unknown))
+      Current.Type = TT_ImplicitStringLiteral;
----------------
Alexander Kornienko wrote:
> Daniel Jasper wrote:
> > Can this actually make a difference? If yes, how? It seems like we convert tok::unknown to tok::string_literal or set Type = TT_ImplicitStringLiteral earlier or merge this into another token's whitespace..
> I think, we want to preserve whitespace around non-whitespace unknown tokens. The most straightforward way to do this seems to be setting the type to TT_ImplicitStringLiteral for the token itself and the next one. The former is done in Format.cpp, and the latter - here.
> 
> And, btw, we don't convert tok::unknown to tok::string_literal in this specific case. I'm not sure if we should.
Why would we want to preserve that whitespace? I'd just add the whitespace as leading whitespace to the TT_ImplicitStringLiteral.

================
Comment at: lib/Format/TokenAnnotator.cpp:575
@@ +574,3 @@
+      Current.Type = TT_ImplicitStringLiteral;
+    if (Current.Type != TT_Unknown)
+      return;
----------------
Alexander Kornienko wrote:
> Daniel Jasper wrote:
> > This change is not a no-op!! Move to line 608 and add:
> > 
> > // FIXME: Add tests that break if this gets moved up.
> > 
> > if no tests are failing now...
> It wasn't meant to be a no-op. This check is here to preserve Current.Type if it is already set to TT_ImplicitStringLiteral. If moving the check here can really break something, we certainly need tests for this. I've added the FIXME line and changed the check to a more specific one.
I think you are actually not achieving what you want to achieve. The code between here and line 614 does not even touch Current's Type. It only changes the type of previous tokens (which might still be bad). However, this check in its current form should not change the behavior. If it does, what is the test case?


http://llvm-reviews.chandlerc.com/D1858