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

[Alexander Kornienko via Phabricator via cfe-commits]

alexfh added inline comments.


================
Comment at: include/clang/Format/Format.h:1676
 
+  /// \brief A vector of macros that should be interpreted as macros expanding
+  /// to a string literal encoding prefix instead of as function calls.
----------------
"A list of macro names"?


================
Comment at: include/clang/Format/Format.h:1686-1689
+  /// These are expected to be macros of the form:
+  /// \code
+  ///   _T("...some string...")
+  /// \endcode
----------------
According to the description above, the option contains "a vector of macros", and here it says "these are expected to be macros of the form ...". This all is rather confusing. The above description should refer to names of macros, and this example should be made clearer in what it demonstrates.


================
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())
----------------
Consider using llvm::find, which is a range adaptor for std::find.


================
Comment at: lib/Format/FormatTokenLexer.cpp:386
   String->HasUnescapedNewline = Macro->HasUnescapedNewline;
+  String->TMacroStringLiteral = true;
 
----------------
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).


https://reviews.llvm.org/D44765