[PATCH] D144115: [clang] Extend pragma dump to support expressions

[Aaron Ballman via Phabricator via cfe-commits]

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