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

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

Szelethus planned changes to this revision.
Szelethus marked an inline comment as done.
Szelethus added a comment.

Thanks! I'll get these fixed.



================
Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:901
+
+  void next(Token &Result) {
+    if (CurrTokenIt == TokenRange.end()) {
----------------
steakhal wrote:
> I don't like output parameters.
> If we really need them, we should at least have a suspicious name.
> Unfortunately, I can not come up with anything useful :|
This is intentional, it meant to replicate the `Lexer`'s interface. I would prefer to keep this as-is.


================
Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:1338-1339
 void TokenPrinter::printToken(const Token &Tok) {
+  // TODO: Handle the case where hash and hashhash occurs right before
+  // __VA_ARGS__.
+
----------------
steakhal wrote:
> What does //hashhash// mean? I might lack some context though :D
`#` and `##` respectively. The test cases you pointed out as flawed refer to this FIXME, though a FIXME in the tests themselves wouldn't hurt.


================
Comment at: clang/test/Analysis/plist-macros-with-expansion.cpp:481
+  i = 0;
+#define DISPATCH(...) PARAMS_RESOLVE_TO_VA_ARGS(__VA_ARGS__);
+
----------------
steakhal wrote:
> You don't need an ending semicolon here. It will be already there at the expansion location.
> This way you introduce an empty expression after the macro expansion.
> The same happens in all the other cases as well.
You are correct, though the point of macro expansion testing is to see whether we nailed what the preprocessor is supposed to do -- not whether the code it creates makes such sense. In fact, I would argue that most GNU extensions to the preprocessor shouldn't be a thing, but we still need to support it.


================
Comment at: clang/test/Analysis/plist-macros-with-expansion.cpp:484-486
+  int x = 1;
+  DISPATCH(x, "LF1M healer");
+  (void)(10 / x); // expected-warning{{Division by zero}}
----------------
steakhal wrote:
> Should we really abuse the division by zero checker here?
> Can't we just use an ExprInspection call here?
> Maybe it requires a specific BugPath visitor, and that is why we do it this way?
We could totally use `ExprInspection` -- but I'd argue that using something else isn't an abuse of the specific checker :) Since the entire file is already written this way, and would demand changes in the large plist file, I'd prefer to keep it this way.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86135