Skip to content

[Clang] Interpreter test should not depend on system header #119903

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

Conversation

yuxuanchen1997
Copy link
Member

@yuxuanchen1997 yuxuanchen1997 commented Dec 13, 2024

30ad53b introduced a new test that includes <vector> from the system include path without honoring environment variables that may provide the path to C++ standard library. This is not supported in some CI systems because we don't always have the C++ library in the standard location.

The conventional way of doing includes in the test is through Inputs directory and pass it as an include path.

The vector file included in this patch has been shortened, but I have verified that it works with this test. i.e. the clang repl crashes on this test in the same way if the fix in #117475 is reverted.

@llvmbot llvmbot added the clang Clang issues not falling into any other category label Dec 13, 2024
@llvmbot
Copy link
Member

llvmbot commented Dec 13, 2024

@llvm/pr-subscribers-clang

Author: Yuxuan Chen (yuxuanchen1997)

Changes

30ad53b introduced a new test that includes <vector> from the system include path without honoring environment variables that may provide the path to C++ standard library. This is not supported in some CI systems because we don't always have a <vector> in standard location.

The conventional way of doing includes in the test is through Inputs directory and pass it as an include path.

The vector file included in this patch has been shortened, but I have verified that it works with this test. i.e. the clang repl crashes on this test in the same way if the fix in #117475 is reverted.


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

2 Files Affected:

  • (added) clang/test/Interpreter/Inputs/vector (+8)
  • (modified) clang/test/Interpreter/crash.cpp (+1-1)
diff --git a/clang/test/Interpreter/Inputs/vector b/clang/test/Interpreter/Inputs/vector
new file mode 100644
index 00000000000000..2add83c58e0d17
--- /dev/null
+++ b/clang/test/Interpreter/Inputs/vector
@@ -0,0 +1,8 @@
+#ifndef VECTOR
+#define VECTOR
+template <typename>
+class vector {
+public:
+  vector();
+};
+#endif // VECTOR
diff --git a/clang/test/Interpreter/crash.cpp b/clang/test/Interpreter/crash.cpp
index 1ab24b0febfa15..9a606983524d82 100644
--- a/clang/test/Interpreter/crash.cpp
+++ b/clang/test/Interpreter/crash.cpp
@@ -5,7 +5,7 @@
 //
 // RUN: split-file %s %t
 //
-// RUN: %clang++ -std=c++20 -fPIC -c %t/vec.cpp -o %t/vec.o
+// RUN: %clang++ -Xclang -nostdsysteminc -I%S/Inputs/ -std=c++20 -fPIC -c %t/vec.cpp -o %t/vec.o
 // RUN: %clang++ -shared %t/vec.o -o %t/vec.so
 //
 // RUN: cat %t/Test.cpp | LD_LIBRARY_PATH=%t:$LD_LIBRARY_PATH clang-repl

@yuxuanchen1997 yuxuanchen1997 force-pushed the users/yuxuanchen1997/clang-interpreter-test-no-sys-header-dep branch from 2ed046e to 449459e Compare December 13, 2024 17:35
@yuxuanchen1997 yuxuanchen1997 changed the title [clang] test should not depend on system header [Clang] Interpreter test should not depend on system header Dec 13, 2024
Copy link
Member

@dcci dcci left a comment

Choose a reason for hiding this comment

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

Thank you for fixing the build

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.

Great catch, apologies for overlooking it. LGTM!

@yuxuanchen1997 yuxuanchen1997 self-assigned this Dec 13, 2024
@yuxuanchen1997 yuxuanchen1997 merged commit 8ab6912 into main Dec 13, 2024
8 checks passed
@yuxuanchen1997 yuxuanchen1997 deleted the users/yuxuanchen1997/clang-interpreter-test-no-sys-header-dep branch December 13, 2024 20:33
dcci added a commit that referenced this pull request Dec 14, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants