[PATCH] D44765: PR36643 Make clang-format support more generic TMarcos

[Alexander Kornienko via Phabricator via cfe-commits]

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