Endill added a comment. Thank you for reviewing this!
In D144115#4147440 <https://reviews.llvm.org/D144115#4147440>, @aaron.ballman wrote: > You should add test coverage for the changes, especially around things like > dependent expressions, ADL use, etc. Also, the changes need a release note > and the new functionality should be explicitly documented. Despite having a dozen of `#pragma __debug` directives, none of them are tested neither mentioned in any kind of documentation, including release notes. Are you sure about this? If so, what is the right place to put them into? ================ Comment at: clang/lib/Parse/ParsePragma.cpp:724-728 + ExprResult E = ParseExpression(); + if (!E.isInvalid()) { + E.get()->dump(); + } + SkipUntil(tok::eod, StopBeforeMatch); ---------------- aaron.ballman wrote: > I think you should have another variant of `ActOnPragmaDump` that does the > actual dumping, instead of doing it from the parser. I think we should not > try to dump a dependent expression (to support those, I think we need to > create a new AST node for the pragma so that we can transform it from > TreeTransform.h and perform the dump on the non-dependent expression -- not > certain if we want to handle that now, or if we want it to be an error to use > this pragma with a dependent expression). > I think you should have another variant of `ActOnPragmaDump` that does the > actual dumping, instead of doing it from the parser. Would it be fine to do it like this: `Actions.ActOnPragmaDump(E)`, where `E` is the result of `ParseExpression()`? > I think we should not try to dump a dependent expression (to support those, I > think we need to create a new AST node for the pragma so that we can > transform it from TreeTransform.h and perform the dump on the non-dependent > expression -- not certain if we want to handle that now, or if we want it to > be an error to use this pragma with a dependent expression). As I mentioned in summary, dependent expressions are out of scope of this patch. How can I test whether expression is dependent? Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D144115/new/ https://reviews.llvm.org/D144115 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
