aaron.ballman added inline comments.
================
Comment at: clang/include/clang/Basic/DiagnosticParseKinds.td:1136-1137
"expected non-wide string literal in '#pragma %0'">, InGroup<IgnoredPragmas>;
+def warn_pragma_expected_ascii_string : Warning<
+ "expected ascii string literal in '#pragma %0'">, InGroup<IgnoredPragmas>;
// - Generic errors
----------------
We use `warn_pragma_expected_non_wide_string` for all the other MS pragmas, so
I think we should use that one here. Then, if we really care to, we can make
`warn_pragma_expected_non_wide_string` more generally about non-ASCII string
literals in a follow up.
================
Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:991-994
+def err_pragma_expected_file_scope : Error<
+ "'#pragma %0' can only appear at file scope">;
+
+def err_pragma_alloc_text_c_linkage: Error<
----------------
================
Comment at: clang/lib/Parse/ParsePragma.cpp:1206
+
+ ExpectAndConsume(tok::r_paren, diag::warn_pragma_expected_rparen,
PragmaName);
+ if (ExpectAndConsume(tok::eof, diag::warn_pragma_extra_tokens_at_eol,
----------------
Should we be returning `false` if this comes back `true`? I'm worried about the
case where there's a missing right paren, we warn about the pragma being
ignored, find the EOL token and then actually handle the pragma.
================
Comment at: clang/lib/Parse/ParsePragma.cpp:1176-1179
+ if (SegmentName->getCharByteWidth() != 1) {
+ PP.Diag(PragmaLocation, diag::warn_pragma_expected_non_wide_string)
+ << PragmaName;
+ return false;
----------------
aaron.ballman wrote:
> I think we want to test `isAscii()` instead of looking at the byte width; we
> don't want to accept `u8"whatever"` any more than we want to accept
> `L"whatever"`.
>
> (The diagnostic text is also pretty bad because it'll diagnose non-wide
> literals as being wide, but that's an existing deficiency with the diagnostic
> and can be addressed separately.)
Ugh, I'm sorry, but I steered you wrong with this comment. I didn't notice
until now that all the other MS pragmas use `getCharByteWidth() != 1` rather
than `isAscii()`. I think we should replicate that mistake here, and then in a
follow-up we can fix all the MS pragmas at once so that they consistently
handle that case. WDYT?
================
Comment at: clang/lib/Sema/SemaAttr.cpp:748
+ &Functions) {
+ if (!CurContext->getRedeclContext()->isFileContext()) {
+ Diag(PragmaLocation, diag::err_pragma_expected_file_scope) << "alloc_text";
----------------
Same question here about whether we need to get the redecl context or not.
================
Comment at: clang/test/Sema/pragma-ms-alloc-text.cpp:5
+#pragma alloc_text(a // expected-warning {{expected ',' in '#pragma
alloc_text'}}
+#pragma alloc_text(a, a // expected-warning {{missing ')' after '#pragma
alloc_text'}} expected-error {{use of undeclared a}}
+#pragma alloc_text(L"a", a) // expected-warning {{expected a string literal
for the section name}}
----------------
This shows we have the parsing logic wrong -- we shouldn't get the use of
undeclared identifier a because we shouldn't have gotten into Sema for this one.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D125011/new/
https://reviews.llvm.org/D125011
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
