================
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:
> > 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?
> I don't completely understand, why the "if (Current.Type == TT_Unknown)"
> check was where it was before, but it's back there now after your comment
> (and I've added the FIXME note you asked me to add), so the semantic of this
> part should be unchanged. What has changed, is the new code that handles the
> case of tok::unknown in the beginning of the function. As this case is quite
> distinct, I don't think it affects anything else. The test for this code is
> in place, and the code really does what it's meant to do (as in "removing the
> code breaks the test").
>
> And I can change the check from the "early exit" style back to what it was
> before, if you prefer it to be that way, btw.
That is plain wrong. I patched this in, commented out this check and all the
tests still pass as I expected.
================
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:
> > 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.
> > Why would we want to preserve that whitespace?
>
> Because it's hard to figure out the meaning of an unknown token. The example
> from http://llvm.org/PR17215 is:
>
> #define VER_QUALIFIER \x20(dbg)
>
> Here we shouldn't insert a space between the backslash and "x20". And if a
> space was there, we shouldn't remove it.
>
> > I'd just add the whitespace as leading whitespace to the
> > TT_ImplicitStringLiteral.
>
> I'm not sure I understand what you mean here. I think, we need to avoid
> touching both leading and trailing whitespace of unknown tokens. And as we
> always attach whitespace to the following token, we need to deal with the
> next token as well. Or you see an easier way? Then could you explain it a bit
> more thoroughly?
But do we really want to preserve all the whitespace? Or would we just want to
preserve that there was whitespace? I'd say we should reformat
#define VER_QUALIFIER \x20(dbg)
to
#define VER_QUALIFIER \x20(dbg)
I think this can easily be achieved by adapting spaceRequiredBefore
appropriately if the left or the right token has the type
TT_ImplicitStringLiteral. Or am I missing something?
http://llvm-reviews.chandlerc.com/D1858
_______________________________________________
cfe-commits mailing list
[email protected]
http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits
