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

[Balázs Benics via Phabricator via cfe-commits]

steakhal added a comment.

I'm not sure about the //status// of this patch.
If you say that further improvements will be done later and this functionality is enough, I'm fine with that.



================
Comment at: clang/lib/StaticAnalyzer/Core/PlistDiagnostics.cpp:901
+
+  void next(Token &Result) {
+    if (CurrTokenIt == TokenRange.end()) {
----------------
Szelethus wrote:
> 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.
Sure, be it.


================
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__.
+
----------------
Szelethus wrote:
> 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.
Maybe `HashtagHashtag`?
Or an example would be even better like: `##__VA_ARGS__`


================
Comment at: clang/test/Analysis/plist-macros-with-expansion.cpp:481
+  i = 0;
+#define DISPATCH(...) PARAMS_RESOLVE_TO_VA_ARGS(__VA_ARGS__);
+
----------------
Szelethus wrote:
> 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.
Oh, now I get it.
I didn't know that this was ann extension lol.


================
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}}
----------------
Szelethus wrote:
> 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.
Perfectly fine. I agree with you knowing this. Thanks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86135