Skip to content

Disable -fdollars-in-identifiers by default #135407

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

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

Disable -fdollars-in-identifiers by default #135407

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
c7e0132
Disable -fdollars-in-identifiers by default
AaronBallman Apr 11, 2025
1e47954
Enable dollar identifiers for unit tests
AaronBallman Apr 11, 2025
72473ef
Switch to compatible langopt
AaronBallman Apr 14, 2025
fb0855f
Update comment based on review feedback; NFC
AaronBallman Apr 15, 2025
b67a2ca
Merge remote-tracking branch 'origin/main' into aballman-dollars-in-i…
AaronBallman Apr 16, 2025
f9b9e81
Add new diagnostic based on review feedback
AaronBallman Apr 16, 2025
e6bc305
Merge remote-tracking branch 'origin/main' into aballman-dollars-in-i…
AaronBallman Apr 17, 2025
8e5b592
Silence the diagnostic in AsmPreprocessor mode
AaronBallman Apr 17, 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
5 changes: 5 additions & 0 deletions clang/docs/ReleaseNotes.rst
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,11 @@ C/C++ Language Potentially Breaking Changes
ensure they are not caught by these optimizations. It is also possible to use
``-fwrapv-pointer`` or ``-fno-delete-null-pointer-checks`` to make pointer arithmetic
on null pointers well-defined. (#GH130734, #GH130742, #GH130952)
- Use of the dollar sign (``$``) in an identifier is no longer a conforming
extension in either C or C++, so ``-fdollars-in-identifiers`` is no longer
enabled by default. Use of the dollar sign in an identifier will now be
diagnosed as an error unless ``-fdollars-in-identifiers`` is explicitly
enabled.

C++ Specific Potentially Breaking Changes
-----------------------------------------
Expand Down
2 changes: 1 addition & 1 deletion clang/include/clang/Basic/LangOptions.def
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,7 @@ LANGOPT(WChar , 1, 0, "wchar_t keyword")
LANGOPT(Char8 , 1, 0, "char8_t keyword")
LANGOPT(IEEE128 , 1, 0, "__ieee128 keyword")
LANGOPT(DeclSpecKeyword , 1, 0, "__declspec keyword")
BENIGN_LANGOPT(DollarIdents , 1, 1, "'$' in identifiers")
COMPATIBLE_LANGOPT(DollarIdents, 1, 0, "'$' in identifiers")
BENIGN_LANGOPT(AsmPreprocessor, 1, 0, "preprocessor in asm mode")
LANGOPT(GNUMode , 1, 1, "GNU extensions")
LANGOPT(GNUKeywords , 1, 1, "GNU keywords")
Expand Down
2 changes: 1 addition & 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 @@ -2088,7 +2088,7 @@ def fno_discard_value_names : Flag<["-"], "fno-discard-value-names">,
Group<f_clang_Group>, Visibility<[ClangOption, DXCOption]>,
HelpText<"Do not discard value names in LLVM IR">;
defm dollars_in_identifiers : BoolFOption<"dollars-in-identifiers",
LangOpts<"DollarIdents">, Default<!strconcat("!", asm_preprocessor.KeyPath)>,
LangOpts<"DollarIdents">, DefaultFalse,
PosFlag<SetTrue, [], [ClangOption], "Allow">,
NegFlag<SetFalse, [], [ClangOption], "Disallow">,
BothFlags<[], [ClangOption, CC1Option], " '$' in identifiers">>;
Expand Down
1 change: 1 addition & 0 deletions clang/lib/Format/Format.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4036,6 +4036,7 @@ LangOptions getFormattingLangOpts(const FormatStyle &Style) {
LangOpts.MicrosoftExt = 1; // To get kw___try, kw___finally.
LangOpts.DeclSpecKeyword = 1; // To get __declspec.
LangOpts.C99 = 1; // To get kw_restrict for non-underscore-prefixed restrict.
LangOpts.DollarIdents = 1; // To allow $ in identifiers.

return LangOpts;
}
Expand Down
2 changes: 1 addition & 1 deletion clang/test/AST/ByteCode/codegen.m
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
/// See test/CodeGenObjC/constant-strings.m
/// Test that we let the APValue we create for ObjCStringLiterals point to the right expression.

// RUN: %clang_cc1 -triple x86_64-macho -emit-llvm -o %t %s -fexperimental-new-constant-interpreter
// RUN: %clang_cc1 -triple x86_64-macho -fdollars-in-identifiers -emit-llvm -o %t %s -fexperimental-new-constant-interpreter
// RUN: FileCheck --check-prefix=CHECK-NEXT < %t %s

// Check that we set alignment 1 on the string.
Expand Down
12 changes: 6 additions & 6 deletions clang/test/C/drs/dr0xx.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,9 +1,9 @@
/* RUN: %clang_cc1 -std=c89 -fsyntax-only -verify=expected,c89only -pedantic -Wno-declaration-after-statement -Wno-c11-extensions %s
RUN: %clang_cc1 -std=c89 -fsyntax-only -verify=expected,c89only -pedantic -Wno-declaration-after-statement -Wno-c11-extensions -fno-signed-char %s
RUN: %clang_cc1 -std=c99 -fsyntax-only -verify=expected,c99untilc2x -pedantic -Wno-c11-extensions %s
RUN: %clang_cc1 -std=c11 -fsyntax-only -verify=expected,c99untilc2x -pedantic %s
RUN: %clang_cc1 -std=c17 -fsyntax-only -verify=expected,c99untilc2x -pedantic %s
RUN: %clang_cc1 -std=c2x -fsyntax-only -verify=expected,c2xandup -pedantic %s
/* RUN: %clang_cc1 -std=c89 -fdollars-in-identifiers -fsyntax-only -verify=expected,c89only -pedantic -Wno-declaration-after-statement -Wno-c11-extensions %s
RUN: %clang_cc1 -std=c89 -fdollars-in-identifiers -fsyntax-only -verify=expected,c89only -pedantic -Wno-declaration-after-statement -Wno-c11-extensions -fno-signed-char %s
RUN: %clang_cc1 -std=c99 -fdollars-in-identifiers -fsyntax-only -verify=expected,c99untilc2x -pedantic -Wno-c11-extensions %s
RUN: %clang_cc1 -std=c11 -fdollars-in-identifiers -fsyntax-only -verify=expected,c99untilc2x -pedantic %s
RUN: %clang_cc1 -std=c17 -fdollars-in-identifiers -fsyntax-only -verify=expected,c99untilc2x -pedantic %s
RUN: %clang_cc1 -std=c2x -fdollars-in-identifiers -fsyntax-only -verify=expected,c2xandup -pedantic %s
*/

/* The following are DRs which do not require tests to demonstrate
Expand Down
2 changes: 1 addition & 1 deletion clang/test/CodeGen/ms-inline-asm.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
// REQUIRES: x86-registered-target
// RUN: %clang_cc1 %s -triple i386-apple-darwin10 -fasm-blocks -emit-llvm -o - | FileCheck %s
// RUN: %clang_cc1 %s -triple i386-apple-darwin10 -fdollars-in-identifiers -fasm-blocks -emit-llvm -o - | FileCheck %s
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just musing... if we wanted this to work automatically, we could implement detection of __asm in the preprocessor, and enable dollars-in-identifiers when lexing inside an __asm block. Detection of __asm at that phase is also necessary to properly handle ; comments, which right now we incorrectly discard after preprocessing. (I'm not sure if there are any other lexing differences for MS __asm that we don't properly handle, but there might be.) But it's probably not worth it for a feature that only works (in MSVC at least) when targeting 32-bit x86. :)

Perhaps a better idea would be to enable -fdollars-in-identifiers under -fms-compatibility, given that MSVC still enables support for dollars in identifiers even in its conforming mode. Though perhaps we can leave that option as a fallback for if someone actually complains.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Perhaps a better idea would be to enable -fdollars-in-identifiers under -fms-compatibility

I thought about this but explicitly decided against it because that means dollars in identifiers is enabled by default for anyone using Clang built by MSVC and targeting Windows. I think we may want to see what folks run into; maybe we want to enable it only for clang-cl and not clang? Maybe we want to be more clever than that? I dunno.

Copy link
Contributor

@R-Goc R-Goc Apr 16, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

maybe we want to enable it only for clang-cl and not clang?

This sounds like an even less expected corner case that would be very annoying.


void t1(void) {
// CHECK: @t1
Expand Down
2 changes: 1 addition & 1 deletion clang/test/CodeGenObjC/extern-void-class-decl.m
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 %s -emit-llvm -o - | FileCheck %s
// RUN: %clang_cc1 -triple x86_64-apple-macosx10.14.0 -fdollars-in-identifiers %s -emit-llvm -o - | FileCheck %s

extern void OBJC_CLASS_$_f;
Class c = (Class)&OBJC_CLASS_$_f;
Expand Down
2 changes: 1 addition & 1 deletion clang/test/Lexer/dollar-idents.c
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
// RUN: %clang_cc1 -dump-tokens %s 2>&1 | FileCheck %s
// RUN: %clang_cc1 -dump-tokens -fdollars-in-identifiers %s 2>&1 | FileCheck %s
// RUN: %clang_cc1 -dump-tokens -x assembler-with-cpp %s 2>&1 | FileCheck %s --check-prefix=CHECK-ASM
// PR3808

Expand Down
17 changes: 17 additions & 0 deletions clang/test/Lexer/gh128939.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
// RUN: %clang_cc1 -E -fdollars-in-identifiers %s 2>&1 | FileCheck %s --check-prefix=CHECK-DOLLARS
// RUN: %clang_cc1 -E %s 2>&1 | FileCheck %s --check-prefix=CHECK-NO-DOLLARS
// GH128939

#define FOO$ 10
#define STR(x) #x
#define STR2(x) STR(x)
const char *p = STR2(FOO$);

// CHECK-NO-DOLLARS: const char *p = "$ 10$";
// CHECK-DOLLARS: const char *p = "10";

#define STR3 STR(
const char *q = STR3$10);

// CHECK-NO-DOLLARS: const char *q = "$10";
// CHECK-DOLLARS: const char *q = STR3$10);
2 changes: 1 addition & 1 deletion clang/test/Preprocessor/c90.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
#define foo`bar /* expected-error {{whitespace required after macro name}} */
#define foo2!bar /* expected-warning {{whitespace recommended after macro name}} */

#define foo3$bar /* expected-error {{'$' in identifier}} */
#define foo3$bar /* expected-error {{whitespace required after macro name}} */
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why does this diagnostic change?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

In -pedentic-errors mode before the patch, dollar signs in identifiers are recognized and so we get # define <identifier> and issue the pedantic diagnostic. With the patch, dollar signs in identifiers are not recognized at all and so we get # define foo $ bar which has no whitespace between the macro name and its replacement list.


/* CHECK-NOT: this comment should be missing
* CHECK: {{^}}// this comment should be present{{$}}
Expand Down
1 change: 1 addition & 0 deletions clang/unittests/Analysis/MacroExpansionContextTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class MacroExpansionContextTest : public ::testing::Test {
TargetOpts->Triple = "x86_64-pc-linux-unknown";
Target = TargetInfo::CreateTargetInfo(Diags, TargetOpts);
LangOpts.CPlusPlus20 = 1; // For __VA_OPT__
LangOpts.DollarIdents = 1; // For $identifier$ testing.
}

IntrusiveRefCntPtr<llvm::vfs::InMemoryFileSystem> InMemoryFileSystem;
Expand Down
Loading