[clang] Remove fixed point arithmetic error #71884
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Prior to this, clang would always report ``` compile with '-ffixed-point' to enable fixed point types ``` whenever it sees `_Accum`, `_Fract`, or `_Sat` when fixed point arithmetic is not enabled. This can break existing code that uses these as variable names and doesn't use fixed point arithmetic like in some microsoft headers (llvm#67750 (comment)). Fixed point should not raise this error for these cases, so this removes the error altogether and defaults to the usual error clang gives where it can see these keywords as either unknown types or regular variables.
llvmbot
added
clang
|
@llvm/pr-subscribers-clang Author: None (PiJoules) ChangesPrior to this, clang would always report whenever it sees Fixed point should not raise this error for these cases, so this removes the error altogether and defaults to the usual error clang gives where it can see these keywords as either unknown types or regular variables. Full diff: https://github.com/llvm/llvm-project/pull/71884.diff 6 Files Affected:
diff --git a/clang/include/clang/Basic/DiagnosticCommonKinds.td b/clang/include/clang/Basic/DiagnosticCommonKinds.td
index 9f0ccd255a32148..1b6b030c86c839e 100644
--- a/clang/include/clang/Basic/DiagnosticCommonKinds.td
+++ b/clang/include/clang/Basic/DiagnosticCommonKinds.td
@@ -260,8 +260,6 @@ def ext_clang_diagnose_if : Extension<"'diagnose_if' is a clang extension">,
InGroup<GccCompat>;
def err_too_large_for_fixed_point : Error<
"this value is too large for this fixed point type">;
-def err_fixed_point_not_enabled : Error<"compile with "
- "'-ffixed-point' to enable fixed point types">;
def err_unimplemented_conversion_with_fixed_point_type : Error<
"conversion between fixed point and %0 is not yet supported">;
diff --git a/clang/include/clang/Basic/TokenKinds.def b/clang/include/clang/Basic/TokenKinds.def
index 3ce317d318f9bb6..b62b2e05fe7dbd3 100644
--- a/clang/include/clang/Basic/TokenKinds.def
+++ b/clang/include/clang/Basic/TokenKinds.def
@@ -284,6 +284,8 @@ PUNCTUATOR(caretcaret, "^^")
// HALFSUPPORT - This is a keyword if 'half' is a built-in type
// WCHARSUPPORT - This is a keyword if 'wchar_t' is a built-in type
// CHAR8SUPPORT - This is a keyword if 'char8_t' is a built-in type
+// KEYFIXEDPOINT - This is a keyword according to the N1169 fixed point
+// extension.
//
KEYWORD(auto , KEYALL)
KEYWORD(break , KEYALL)
@@ -423,9 +425,9 @@ C23_KEYWORD(typeof , KEYGNU)
C23_KEYWORD(typeof_unqual , 0)
// ISO/IEC JTC1 SC22 WG14 N1169 Extension
-KEYWORD(_Accum , KEYNOCXX)
-KEYWORD(_Fract , KEYNOCXX)
-KEYWORD(_Sat , KEYNOCXX)
+KEYWORD(_Accum , KEYFIXEDPOINT)
+KEYWORD(_Fract , KEYFIXEDPOINT)
+KEYWORD(_Sat , KEYFIXEDPOINT)
// GNU Extensions (in impl-reserved namespace)
KEYWORD(_Decimal32 , KEYALL)
diff --git a/clang/lib/Basic/IdentifierTable.cpp b/clang/lib/Basic/IdentifierTable.cpp
index c4c5a6eeced2832..38150d1640d709d 100644
--- a/clang/lib/Basic/IdentifierTable.cpp
+++ b/clang/lib/Basic/IdentifierTable.cpp
@@ -108,7 +108,8 @@ namespace {
KEYSYCL = 0x800000,
KEYCUDA = 0x1000000,
KEYHLSL = 0x2000000,
- KEYMAX = KEYHLSL, // The maximum key
+ KEYFIXEDPOINT = 0x4000000,
+ KEYMAX = KEYFIXEDPOINT, // The maximum key
KEYALLCXX = KEYCXX | KEYCXX11 | KEYCXX20,
KEYALL = (KEYMAX | (KEYMAX-1)) & ~KEYNOMS18 &
~KEYNOOPENCL // KEYNOMS18 and KEYNOOPENCL are used to exclude.
@@ -210,6 +211,8 @@ static KeywordStatus getKeywordStatusHelper(const LangOptions &LangOpts,
case KEYNOMS18:
// The disable behavior for this is handled in getKeywordStatus.
return KS_Unknown;
+ case KEYFIXEDPOINT:
+ return LangOpts.FixedPoint ? KS_Enabled : KS_Disabled;
default:
llvm_unreachable("Unknown KeywordStatus flag");
}
diff --git a/clang/lib/Parse/ParseDecl.cpp b/clang/lib/Parse/ParseDecl.cpp
index 78c3ab72979a007..06f2c8798b049f8 100644
--- a/clang/lib/Parse/ParseDecl.cpp
+++ b/clang/lib/Parse/ParseDecl.cpp
@@ -3288,17 +3288,6 @@ Parser::DiagnoseMissingSemiAfterTagDefinition(DeclSpec &DS, AccessSpecifier AS,
return false;
}
-// Choose the apprpriate diagnostic error for why fixed point types are
-// disabled, set the previous specifier, and mark as invalid.
-static void SetupFixedPointError(const LangOptions &LangOpts,
- const char *&PrevSpec, unsigned &DiagID,
- bool &isInvalid) {
- assert(!LangOpts.FixedPoint);
- DiagID = diag::err_fixed_point_not_enabled;
- PrevSpec = ""; // Not used by diagnostic
- isInvalid = true;
-}
-
/// ParseDeclarationSpecifiers
/// declaration-specifiers: [C99 6.7]
/// storage-class-specifier declaration-specifiers[opt]
@@ -4275,27 +4264,24 @@ void Parser::ParseDeclarationSpecifiers(
DiagID, Policy);
break;
case tok::kw__Accum:
- if (!getLangOpts().FixedPoint) {
- SetupFixedPointError(getLangOpts(), PrevSpec, DiagID, isInvalid);
- } else {
- isInvalid = DS.SetTypeSpecType(DeclSpec::TST_accum, Loc, PrevSpec,
- DiagID, Policy);
- }
+ assert(getLangOpts().FixedPoint &&
+ "This keyword is only used when fixed point types are enabled "
+ "with `-ffixed-point`");
+ isInvalid = DS.SetTypeSpecType(DeclSpec::TST_accum, Loc, PrevSpec, DiagID,
+ Policy);
break;
case tok::kw__Fract:
- if (!getLangOpts().FixedPoint) {
- SetupFixedPointError(getLangOpts(), PrevSpec, DiagID, isInvalid);
- } else {
- isInvalid = DS.SetTypeSpecType(DeclSpec::TST_fract, Loc, PrevSpec,
- DiagID, Policy);
- }
+ assert(getLangOpts().FixedPoint &&
+ "This keyword is only used when fixed point types are enabled "
+ "with `-ffixed-point`");
+ isInvalid = DS.SetTypeSpecType(DeclSpec::TST_fract, Loc, PrevSpec, DiagID,
+ Policy);
break;
case tok::kw__Sat:
- if (!getLangOpts().FixedPoint) {
- SetupFixedPointError(getLangOpts(), PrevSpec, DiagID, isInvalid);
- } else {
- isInvalid = DS.SetTypeSpecSat(Loc, PrevSpec, DiagID);
- }
+ assert(getLangOpts().FixedPoint &&
+ "This keyword is only used when fixed point types are enabled "
+ "with `-ffixed-point`");
+ isInvalid = DS.SetTypeSpecSat(Loc, PrevSpec, DiagID);
break;
case tok::kw___float128:
isInvalid = DS.SetTypeSpecType(DeclSpec::TST_float128, Loc, PrevSpec,
diff --git a/clang/test/Frontend/fixed_point_as_variables.c b/clang/test/Frontend/fixed_point_as_variables.c
new file mode 100644
index 000000000000000..1eb969a355d3b72
--- /dev/null
+++ b/clang/test/Frontend/fixed_point_as_variables.c
@@ -0,0 +1,11 @@
+// RUN: %clang_cc1 -x c -verify %s
+// RUN: %clang_cc1 -x c -verify %s -ffixed-point -DFIXED_POINT=1
+
+int _Accum;
+
+#ifdef FIXED_POINT
+// expected-error@4{{cannot combine with previous 'int' declaration specifier}}
+// expected-warning@4{{declaration does not declare anything}}
+#else
+// expected-no-diagnostics
+#endif
diff --git a/clang/test/Frontend/fixed_point_not_enabled.c b/clang/test/Frontend/fixed_point_not_enabled.c
index a1a60c5a6fa8007..c381f1c8f1835a1 100644
--- a/clang/test/Frontend/fixed_point_not_enabled.c
+++ b/clang/test/Frontend/fixed_point_not_enabled.c
@@ -1,18 +1,14 @@
// RUN: %clang_cc1 -x c -verify %s
// Primary fixed point types
-signed short _Accum s_short_accum; // expected-error{{compile with '-ffixed-point' to enable fixed point types}}
-signed _Accum s_accum; // expected-error{{compile with '-ffixed-point' to enable fixed point types}}
-signed long _Accum s_long_accum; // expected-error{{compile with '-ffixed-point' to enable fixed point types}}
-unsigned short _Accum u_short_accum; // expected-error{{compile with '-ffixed-point' to enable fixed point types}}
-unsigned _Accum u_accum; // expected-error{{compile with '-ffixed-point' to enable fixed point types}}
-unsigned long _Accum u_long_accum; // expected-error{{compile with '-ffixed-point' to enable fixed point types}}
-
-// Aliased fixed point types
-short _Accum short_accum; // expected-error{{compile with '-ffixed-point' to enable fixed point types}}
-_Accum accum; // expected-error{{compile with '-ffixed-point' to enable fixed point types}}
- // expected-error@-1{{type specifier missing, defaults to 'int'}}
-long _Accum long_accum; // expected-error{{compile with '-ffixed-point' to enable fixed point types}}
+// Without `-ffixed-point`, these keywords are now treated as typedef'd types or identifiers.
+_Accum accum; // expected-error{{unknown type name '_Accum'}}
+_Fract fract; // expected-error{{unknown type name '_Fract'}}
+_Sat _Accum sat_accum; // expected-error{{unknown type name '_Sat'}}
+ // expected-error@-1{{expected ';' after top level declarator}}
+signed _Accum signed_accum; // expected-error{{expected ';' after top level declarator}}
+signed _Fract signed_fract; // expected-error{{expected ';' after top level declarator}}
+signed _Sat _Accum signed_sat_accum; // expected-error{{expected ';' after top level declarator}}
// Cannot use fixed point suffixes
int accum_int = 10k; // expected-error{{invalid suffix 'k' on integer constant}}
|
You can test this locally with the following command:git-clang-format --diff ea89965b3cfcb00a08941780da45c663cb4a6cab b1263d73347612acd2109a88c1188054e29e0082 -- clang/test/Frontend/fixed_point_as_variables.c clang/lib/Basic/IdentifierTable.cpp clang/lib/Parse/ParseDecl.cpp clang/test/Frontend/fixed_point_not_enabled.cView the diff from clang-format here.diff --git a/clang/lib/Basic/IdentifierTable.cpp b/clang/lib/Basic/IdentifierTable.cpp
index 38150d1640d7..56413b7197ac 100644
--- a/clang/lib/Basic/IdentifierTable.cpp
+++ b/clang/lib/Basic/IdentifierTable.cpp
@@ -81,49 +81,49 @@ IdentifierTable::IdentifierTable(const LangOptions &LangOpts,
// Constants for TokenKinds.def
namespace {
- enum TokenKey : unsigned {
- KEYC99 = 0x1,
- KEYCXX = 0x2,
- KEYCXX11 = 0x4,
- KEYGNU = 0x8,
- KEYMS = 0x10,
- BOOLSUPPORT = 0x20,
- KEYALTIVEC = 0x40,
- KEYNOCXX = 0x80,
- KEYBORLAND = 0x100,
- KEYOPENCLC = 0x200,
- KEYC23 = 0x400,
- KEYNOMS18 = 0x800,
- KEYNOOPENCL = 0x1000,
- WCHARSUPPORT = 0x2000,
- HALFSUPPORT = 0x4000,
- CHAR8SUPPORT = 0x8000,
- KEYOBJC = 0x10000,
- KEYZVECTOR = 0x20000,
- KEYCOROUTINES = 0x40000,
- KEYMODULES = 0x80000,
- KEYCXX20 = 0x100000,
- KEYOPENCLCXX = 0x200000,
- KEYMSCOMPAT = 0x400000,
- KEYSYCL = 0x800000,
- KEYCUDA = 0x1000000,
- KEYHLSL = 0x2000000,
- KEYFIXEDPOINT = 0x4000000,
- KEYMAX = KEYFIXEDPOINT, // The maximum key
- KEYALLCXX = KEYCXX | KEYCXX11 | KEYCXX20,
- KEYALL = (KEYMAX | (KEYMAX-1)) & ~KEYNOMS18 &
- ~KEYNOOPENCL // KEYNOMS18 and KEYNOOPENCL are used to exclude.
- };
-
- /// How a keyword is treated in the selected standard. This enum is ordered
- /// intentionally so that the value that 'wins' is the most 'permissive'.
- enum KeywordStatus {
- KS_Unknown, // Not yet calculated. Used when figuring out the status.
- KS_Disabled, // Disabled
- KS_Future, // Is a keyword in future standard
- KS_Extension, // Is an extension
- KS_Enabled, // Enabled
- };
+enum TokenKey : unsigned {
+ KEYC99 = 0x1,
+ KEYCXX = 0x2,
+ KEYCXX11 = 0x4,
+ KEYGNU = 0x8,
+ KEYMS = 0x10,
+ BOOLSUPPORT = 0x20,
+ KEYALTIVEC = 0x40,
+ KEYNOCXX = 0x80,
+ KEYBORLAND = 0x100,
+ KEYOPENCLC = 0x200,
+ KEYC23 = 0x400,
+ KEYNOMS18 = 0x800,
+ KEYNOOPENCL = 0x1000,
+ WCHARSUPPORT = 0x2000,
+ HALFSUPPORT = 0x4000,
+ CHAR8SUPPORT = 0x8000,
+ KEYOBJC = 0x10000,
+ KEYZVECTOR = 0x20000,
+ KEYCOROUTINES = 0x40000,
+ KEYMODULES = 0x80000,
+ KEYCXX20 = 0x100000,
+ KEYOPENCLCXX = 0x200000,
+ KEYMSCOMPAT = 0x400000,
+ KEYSYCL = 0x800000,
+ KEYCUDA = 0x1000000,
+ KEYHLSL = 0x2000000,
+ KEYFIXEDPOINT = 0x4000000,
+ KEYMAX = KEYFIXEDPOINT, // The maximum key
+ KEYALLCXX = KEYCXX | KEYCXX11 | KEYCXX20,
+ KEYALL = (KEYMAX | (KEYMAX - 1)) & ~KEYNOMS18 &
+ ~KEYNOOPENCL // KEYNOMS18 and KEYNOOPENCL are used to exclude.
+};
+
+/// How a keyword is treated in the selected standard. This enum is ordered
+/// intentionally so that the value that 'wins' is the most 'permissive'.
+enum KeywordStatus {
+ KS_Unknown, // Not yet calculated. Used when figuring out the status.
+ KS_Disabled, // Disabled
+ KS_Future, // Is a keyword in future standard
+ KS_Extension, // Is an extension
+ KS_Enabled, // Enabled
+};
} // namespace
|
petrhosek
approved these changes
Nov 13, 2023
|
I'm happy with the basic idea here of making fixed-point an opt-in feature. It'd be nice to preserve the special diagnostic that tells the user that they need to use |
Yeah, although from what I've seen, there's a lot of unrecognized identifier paths. The order of operations seems to be (1) determine what keywords are allowed, then (2) lex allowed keywords and set the appropriate properties on tokens, then (3) handle the token based on if it's an identifier or keyword. What I initially wanted was something where I'd be fine with doing either in a later patch and making a TODO issue for improving the diagnostics. |
|
I definitely don't think we should be handling this in the lexer by trying to retroactively make this a keyword. I was just thinking that we might have some sort of parser-level recovery for e.g. |
zahiraam
pushed a commit
to zahiraam/llvm-project
that referenced
this pull request
Nov 20, 2023
Prior to this, clang would always report ``` compile with '-ffixed-point' to enable fixed point types ``` whenever it sees `_Accum`, `_Fract`, or `_Sat` when fixed point arithmetic is not enabled. This can break existing code that uses these as variable names and doesn't use fixed point arithmetic like in some microsoft headers (llvm#67750 (comment)). Fixed point should not raise this error for these cases, so this removes the error altogether and defaults to the usual error clang gives where it can see these keywords as either unknown types or regular variables.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Prior to this, clang would always report
whenever it sees
_Accum,_Fract, or_Satwhen fixed point arithmetic is not enabled. This can break existing code that uses these as variable names and doesn't use fixed point arithmetic like in some microsoft headers(#67750 (comment)).
Fixed point should not raise this error for these cases, so this removes the error altogether and defaults to the usual error clang gives where it can see these keywords as either unknown types or regular variables.