alexfh added inline comments.
================
Comment at: lib/Format/FormatTokenLexer.cpp:371
FormatToken *Macro = Tokens[Tokens.size() - 4];
- if (Macro->TokenText != "_T")
+ if (std::find(Style.TMacros.begin(), Style.TMacros.end(), Macro->TokenText)
==
+ Style.TMacros.end())
----------------
obfuscated wrote:
> alexfh wrote:
> > Consider using llvm::find, which is a range adaptor for std::find.
> No idea what is range adaptor, but I can probably switch to it... :)
It works with a range (something that has .begin() and .end()) instead of two
iterators.
================
Comment at: lib/Format/FormatTokenLexer.cpp:386
String->HasUnescapedNewline = Macro->HasUnescapedNewline;
+ String->TMacroStringLiteral = true;
----------------
obfuscated wrote:
> alexfh wrote:
> > obfuscated wrote:
> > > krasimir wrote:
> > > > In the original code, TMacro detection was done as:
> > > > ```
> > > > (Text.startswith(Prefix = "_T(\"") && Text.endswith(Postfix = "\")"))
> > > > ```
> > > > In the new code the left part is saved in TMacroStringLiteral, and the
> > > > right part is checked in ContinuationIndenter. Could you keep them
> > > > together in `FormatTokenLexer`.
> > > > @alexfh, why are we checking for the right check at all? What would be
> > > > an example where this is needed to disambiguate?
> > > >
> > > Are you talking about the code in
> > > ContinuationIndenter::createBreakableToken?
> > >
> > > I don't think I understand how the hole thing works.
> > > Using the debugger I can see that this function is executed first and
> > > then createBreakableToken.
> > > So we first detect the tmacro in FormatTokenLexer::tryMerge_TMacro and
> > > then use this information in the createBreakableToken to do something
> > > with it.
> > >
> > > So when I get a TMacroStringLiteral==true in createBreakableToken I know
> > > that the token starts with a TMacro and there is no need to use some king
> > > of startswith calls. Probably the endswith call is redundant and I can do
> > > just a string search backwards...
> > > In the new code the left part is saved in TMacroStringLiteral, and the
> > > right part is checked in ContinuationIndenter. Could you keep them
> > > together in FormatTokenLexer.
> >
> > +1 to keeping these conditions together.
> >
> > > @alexfh, why are we checking for the right check at all? What would be an
> > > example where this is needed to disambiguate?
> >
> > I don't know whether there's reasonable code that would be handled
> > incorrectly without checking for the `")`, but I'm sure some test case can
> > be generated, that would break clang-format badly without this condition.
> > It also serves as a documentation (similar to asserts).
> >> In the new code the left part is saved in TMacroStringLiteral, and the
> >> right part is checked in ContinuationIndenter. Could you keep them
> >> together in FormatTokenLexer.
> >
> > +1 to keeping these conditions together.
>
> Can you please explain what you mean here?
> And what am I supposed to do, because I don't know how to put these
> conditions together.
> Do you want me to search the tmacro vector in
> ContinuationIndenter::createBreakableToken instead of checking the bool flag?
>
>
>
Looking at this code again, I see what the checks for `_T("` and `")` were
needed for. If there was a space between `_T(` and `"` or between `"` and `)`,
the code in `createBreakableToken()` would probably not work correctly (see the
FIXME). `tryMerge_TMacro()` looks at tokens and doesn't care about the
whitespace, so checking for `TMacroStringLiteral` is not equivalent to checking
for `<macro-name>("` and `")`. In particular, if you only remove just the first
check, a test case like `_T ( "...")` may start being formatted in some way.
We could remove the second check as well, if we manage to make formatting
decent. Otherwise, performing these checks (that there are no spaces between
`_T`, `(`, `"..."` and `)`) in `tryMerge_TMacro()` may be a better option than
doing this in `createBreakableToken()`.
https://reviews.llvm.org/D44765
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
