Skip to content
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.

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

[clang-repl] Fix Value for platforms where unqualified char is unsigned #86118

Conversation

weliveindetail
Copy link
Contributor

Signedness of unqualified char is unspecified and varies between platforms. This patch adds Char_U in REPL_BUILTIN_TYPES to account for platforms that default to unsigned char.

@llvmbot llvmbot added clang Clang issues not falling into any other category clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Mar 21, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented Mar 21, 2024

@llvm/pr-subscribers-clang

Author: Stefan Gränitz (weliveindetail)

Changes

Signedness of unqualified char is unspecified and varies between platforms. This patch adds Char_U in REPL_BUILTIN_TYPES to account for platforms that default to unsigned char.


Full diff: https://github.com/llvm/llvm-project/pull/86118.diff

2 Files Affected:

  • (modified) clang/include/clang/Interpreter/Value.h (+1)
  • (modified) clang/unittests/Interpreter/InterpreterTest.cpp (+6)
diff --git a/clang/include/clang/Interpreter/Value.h b/clang/include/clang/Interpreter/Value.h
index c380cd91550def..d70e8f8719026b 100644
--- a/clang/include/clang/Interpreter/Value.h
+++ b/clang/include/clang/Interpreter/Value.h
@@ -76,6 +76,7 @@ class QualType;
   X(bool, Bool)                                                                \
   X(char, Char_S)                                                              \
   X(signed char, SChar)                                                        \
+  X(unsigned char, Char_U)                                                     \
   X(unsigned char, UChar)                                                      \
   X(short, Short)                                                              \
   X(unsigned short, UShort)                                                    \
diff --git a/clang/unittests/Interpreter/InterpreterTest.cpp b/clang/unittests/Interpreter/InterpreterTest.cpp
index e76c0677db5ead..4b5d73769e5da7 100644
--- a/clang/unittests/Interpreter/InterpreterTest.cpp
+++ b/clang/unittests/Interpreter/InterpreterTest.cpp
@@ -340,6 +340,12 @@ TEST(InterpreterTest, Value) {
   EXPECT_EQ(V1.getKind(), Value::K_Int);
   EXPECT_FALSE(V1.isManuallyAlloc());
 
+  Value V1b;
+  llvm::cantFail(Interp->ParseAndExecute("char x = 42;"));
+  llvm::cantFail(Interp->ParseAndExecute("c", &V1b));
+  EXPECT_TRUE(V1b.getKind() == Value::K_Char_S ||
+              V1b.getKind() == Value::K_Char_U);
+
   Value V2;
   llvm::cantFail(Interp->ParseAndExecute("double y = 3.14;"));
   llvm::cantFail(Interp->ParseAndExecute("y", &V2));

Copy link
Contributor

@vgvassilev vgvassilev left a comment

Choose a reason for hiding this comment

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

Good catch! Thank you, @weliveindetail!

@weliveindetail weliveindetail force-pushed the clang-repl-add-unqualified-char-unsigned branch from 3ade711 to 7db25a9 Compare March 22, 2024 11:24
@weliveindetail
Copy link
Contributor Author

Let's give the pre-merge checks a 2nd chance..

@weliveindetail
Copy link
Contributor Author

Windows buildbot failure is unrelated. This code is covered in unittest ClangReplInterpreterTests and it passed.

@weliveindetail weliveindetail merged commit aa962d6 into llvm:main Mar 25, 2024
3 of 4 checks passed
@weliveindetail weliveindetail deleted the clang-repl-add-unqualified-char-unsigned branch March 25, 2024 08:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants