Skip to content

[Clang] Ignore -fchar8_t in C #138716

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 2 commits into from
May 8, 2025
Merged

[Clang] Ignore -fchar8_t in C #138716

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb8c804
[Clang] Ignore -fchar8_t in C
cor3ntin May 6, 2025
8536748
typo
cor3ntin May 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -317,6 +317,8 @@ Modified Compiler Flags

- The ``-mexecute-only`` and ``-mpure-code`` flags are now accepted for AArch64 targets. (#GH125688)

- The ``-fchar8_t`` flag is no longer consider in non-C++ languages modes. (#GH55373)
Copy link
Collaborator

@AaronBallman AaronBallman May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- The ``-fchar8_t`` flag is no longer consider in non-C++ languages modes. (#GH55373)
- The ``-fchar8_t`` flag is now diagnosed in non-C++ languages modes. (#GH55373)


Removed Compiler Flags
-------------------------

Expand Down
3 changes: 2 additions & 1 deletion clang/include/clang/Driver/Options.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3879,7 +3879,8 @@ defm char8__t : BoolFOption<"char8_t",
LangOpts<"Char8">, Default<cpp20.KeyPath>,
PosFlag<SetTrue, [], [ClangOption], "Enable">,
NegFlag<SetFalse, [], [ClangOption], "Disable">,
BothFlags<[], [ClangOption, CC1Option], " C++ builtin type char8_t">>;
BothFlags<[], [ClangOption, CC1Option], " C++ builtin type char8_t">>,
ShouldParseIf<cplusplus.KeyPath>;
def fshort_wchar : Flag<["-"], "fshort-wchar">, Group<f_Group>,
HelpText<"Force wchar_t to be a short unsigned int">;
def fno_short_wchar : Flag<["-"], "fno-short-wchar">, Group<f_Group>,
Expand Down
1 change: 1 addition & 0 deletions clang/test/Lexer/char8_t.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
// RUN: %clang_cc1 -std=c++17 -verify %s
// RUN: %clang_cc1 -std=c++17 -verify %s -fno-char8_t
// RUN: %clang_cc1 -std=c++20 -verify %s -fno-char8_t
// RUN: %clang_cc1 -x c -verify %s -fchar8_t
Copy link
Collaborator

@AaronBallman AaronBallman May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Shouldn't this generate a diagnostic now? Or are we planning to continue to accept in cc1 mode? If so, we need a driver test showing we reject from the driver side.

Copy link
Contributor Author

@cor3ntin cor3ntin May 6, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ShouldParseIf seems never to diagnose. I wanted to ask you about that because it might be surprising
https://godbolt.org/z/6oaPfoWPj

Copy link
Collaborator

@AaronBallman AaronBallman May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CC @jansvoboda11 @MaskRay for more opinions as driver and options maintainers.

I guess I find that behavior kind of surprising. I would expect "you passed this flag and this flag does nothing" should at least be a warning. It's a bit different from an unknown flag, but the same general logic applies: the user passed something and we either know about it and explicitly don't do anything with it, or we don't know about it and don't do anything with it, but either way it seems like the user should be told "this was unknown".

Copy link
Contributor Author

@cor3ntin cor3ntin May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree, but I think it's preexisting / orthogonal

Copy link
Collaborator

@AaronBallman AaronBallman May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yup, it is! Only action item I expect out of that is filing an issue if the options folks agree.

Copy link
Contributor

@jansvoboda11 jansvoboda11 May 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Filing an issues sounds good to me. I don't think great diagnostics for -cc1 flags are a high priority, but improving them certainly doesn't hurt!

Copy link
Member

@MaskRay MaskRay May 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we want to emit a warning, in RenderCharacterOptions, we cannot unconditionally claim the Arg, but only claim it when the language mode is C++. I have some notes: https://maskray.me/blog/2023-08-25-clang-wunused-command-line-argument

Copy link
Contributor Author

@cor3ntin cor3ntin May 8, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#139058


#if defined(__cpp_char8_t) != defined(CHAR8_T)
#error wrong setting for __cpp_char_t
Expand Down