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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
