================
@@ -1362,20 +1368,30 @@ void Preprocessor::HandleDirective(Token &Result) {
case tok::pp_module:
case tok::pp___preprocessed_module:
case tok::pp___preprocessed_import:
- Diag(Result, diag::err_embedded_directive)
- << (getLangOpts().CPlusPlusModules &&
- Introducer.isModuleContextualKeyword(
- /*AllowExport=*/false))
- << II->getName();
- Diag(*ArgMacro, diag::note_macro_expansion_here)
- << ArgMacro->getIdentifierInfo();
- DiscardUntilEndOfDirective();
- return;
+ return true;
default:
- break;
+ return false;
}
+ };
+
+ // [cpp.replace.general] makes any embedded directive ill-formed in
+ // C++26; in earlier modes the construct is undefined behavior and only
+ // pedantically warned about.
+ const bool IsError = LangOpts.CPlusPlus26 ||
+ (II && IsAlwaysUnsupported(II->getPPKeywordID()));
+
----------------
AaronBallman wrote:
I think it's reasonable as an error in all language modes but I don't think the
changes here are quite correct. This is implementing
https://eel.is/c++draft/cpp.replace#general-14.sentence-3 which has similar
wording in C (undefined rather than requiring a diagnostic), and that's not
dependent on the keyword. e.g., this should also be rejected:
```
#define FUNCTION_MACRO(...)
FUNCTION_MACRO(
#
)
```
because it's still a directive. I think the only time we need to look at
identifier are the cases which don't start with a leading `#` here:
https://eel.is/c++draft/cpp#pre-2 (because we already diagnose non-directives
as an error: https://godbolt.org/z/s1dY3Kc4T) But those special cases also
aren't handled correctly because this should be accepted (doesn't match the
requirements to make it a directive):
```
#define FUNCTION_MACRO(...)
FUNCTION_MACRO(
module
)
```
but I don't think we even get into `HandleDirective` for the cases that don't
start with `#` because we don't warn currently: https://godbolt.org/z/qKjjdr8sx
So I think there's a little bit more work to be done getting the diagnostics
just right, but I think it's reasonable to diagnose as an error in C as well as
C++. (Maybe drop the embedded directive changes from this PR so we can land it
and then do those in a separate PR?)
https://github.com/llvm/llvm-project/pull/192073
_______________________________________________
cfe-commits mailing list
[email protected]
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
