aaron.ballman added a comment.
In D144115#4148026 <https://reviews.llvm.org/D144115#4148026>, @Endill wrote:
> 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?
Yeah, we've been very bad about documenting our implementation-defined
behaviors and I've been on a push to change that moving forward for about the
past year or so. You don't have to document all of the debug pragmas by any
means, but getting some basic documentation up for the pragma being worked on
is good incremental progress. I'd recommend adding the tests to
`clang/test/Preprocessor` and adding the documentation to
`clang\docs\LanguageExtensions.rst`.
================
Comment at: clang/lib/Parse/ParsePragma.cpp:724-728
+ ExprResult E = ParseExpression();
+ if (!E.isInvalid()) {
+ E.get()->dump();
+ }
+ SkipUntil(tok::eod, StopBeforeMatch);
----------------
Endill wrote:
> 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?
>
>
>
>>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 it'd make sense to do `Actions.ActOnPragmaDump(Tok.getLocation(),
ParseExpression());` so that we have the location of the pragma token itself in
addition to the expression result for the argument.
> As I mentioned in summary, dependent expressions are out of scope of this
> patch. How can I test whether expression is dependent?
`Expr::isValueDependent()` and `Expr::isTypeDependent()`
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
