[PATCH] D86135: [analyzer][MacroExpansion] Fix a crash where multiple parameters resolved to __VA_ARGS__

[Kristóf Umann via Phabricator via cfe-commits]

Szelethus added inline comments.


================
Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:896-898
+    RawLexer.reset(new Lexer(SM.getLocForStartOfFile(LocInfo.first), LangOpts,
+                             MB->getBufferStart(), MacroNameTokenPos,
+                             MB->getBufferEnd()));
----------------
steakhal wrote:
> I'm always puzzled if I see a naked `new`.
> Couldn't we use the assignment operator and `std::make_unique` here?
Wait, isn't it naked if its //not// surrounded by smart pointer stuff? In any case, explicit calls to operator `new` and `delete` are indeed discouraged by the [[http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#r11-avoid-calling-new-and-delete-explicitly | core guidelines]].


================
Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:1150-1161
   // Acquire the macro's arguments at the expansion point.
   //
   // The rough idea here is to lex from the first left parentheses to the last
   // right parentheses, and map the macro's parameter to what they will be
   // expanded to. A macro argument may contain several token (like '3 + 4'), so
   // we'll lex until we find a tok::comma or tok::r_paren, at which point we
   // start lexing the next argument or finish.
----------------
steakhal wrote:
> By //lexing// one might think we use the actual lexer. Should we change this comment?
I see what you mean, but this is why its phrased as a rough idea, not an in-depth step-by-step description of the process covering corner cases -- not in this comment, at least. Functionally, you could argue that we're lexing even if we get tokens from the injected range.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86135/new/

https://reviews.llvm.org/D86135