Skip to content

Make -DUSE_REPL=On default #360

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 12 commits into from
Dec 17, 2024
Merged

Make -DUSE_REPL=On default #360

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
15321dc
Make USE_REPL=On
faze-geek Nov 29, 2024
bffb355
Remove unnecessary DUSE_REPL=ON tags
faze-geek Nov 29, 2024
233d178
Update InterpreterTest.cpp
faze-geek Nov 29, 2024
5b55c3d
Update Compatibility.h
faze-geek Nov 29, 2024
d3752df
Add USE_CLING definition instead of USE_REPL
faze-geek Dec 9, 2024
039332f
Merge branch 'main' into change_cmake
faze-geek Dec 9, 2024
1e18aa7
Remove unnecessary space
faze-geek Dec 9, 2024
a66d6ce
Update ifdef REPL to ifndef CLING
faze-geek Dec 14, 2024
8867cf5
Minor fix
faze-geek Dec 14, 2024
9432bb5
Add missed endif
faze-geek Dec 14, 2024
8f048f5
Merge branch 'main' into change_cmake
faze-geek Dec 14, 2024
cb6a3a0
Merge branch 'main' into change_cmake
faze-geek Dec 17, 2024
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
3 changes: 0 additions & 3 deletions .github/workflows/ci.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1191,7 +1191,6 @@ jobs:
else
cmake -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} \
-DUSE_CLING=OFF \
Copy link
Collaborator

Choose a reason for hiding this comment

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

@faze-geek If use_repl has been made the default then we should be able to remove all the uses of use_cling=off, since it should be off by default.

-DUSE_REPL=ON \
-DCPPINTEROP_INCLUDE_DOCS=${{ matrix.documentation }} \
-DLLVM_DIR=$LLVM_BUILD_DIR/lib/cmake/llvm \
-DClang_DIR=$LLVM_BUILD_DIR/lib/cmake/clang \
Expand Down Expand Up @@ -1285,7 +1284,6 @@ jobs:
{
cmake -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} `
-DUSE_CLING=OFF `
-DUSE_REPL=ON `
-DLLVM_DIR="$env:LLVM_BUILD_DIR\lib\cmake\llvm" `
-DLLVM_ENABLE_WERROR=On `
-DClang_DIR="$env:LLVM_BUILD_DIR\lib\cmake\clang" -DCODE_COVERAGE=${{ env.CODE_COVERAGE }} -DCMAKE_INSTALL_PREFIX="$env:CPPINTEROP_DIR" ..\
Expand Down Expand Up @@ -1580,7 +1578,6 @@ jobs:
else
emcmake cmake -DCMAKE_BUILD_TYPE=${{ env.BUILD_TYPE }} \
-DUSE_CLING=OFF \
-DUSE_REPL=ON \
Copy link
Collaborator

Choose a reason for hiding this comment

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

please make the similar changes in this file too

https://github.com/compiler-research/CppInterOp/blob/main/.github/workflows/deploy-pages.yml

Copy link
Collaborator

Choose a reason for hiding this comment

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

I suggest he goes through all workflows in the repo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for letting me know.

-DCMAKE_PREFIX_PATH=$PREFIX \
-DLLVM_DIR=$LLVM_BUILD_DIR/lib/cmake/llvm \
-DLLD_DIR=$LLVM_BUILD_DIR/lib/cmake/lld \
Expand Down
2 changes: 1 addition & 1 deletion CMakeLists.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@ if( CMAKE_SOURCE_DIR STREQUAL CMAKE_CURRENT_SOURCE_DIR )
endif()

option(USE_CLING "Use Cling as backend" OFF)
option(USE_REPL "Use clang-repl as backend" OFF)
option(USE_REPL "Use clang-repl as backend" ON)
Copy link
Collaborator

Choose a reason for hiding this comment

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

As the block below points out the following

  if (USE_CLING AND USE_REPL)
    message(FATAL_ERROR "We can only use Cling (USE_CLING=On) or Repl (USE_REPL=On), but not both of them.")
  endif()

What can be done if we are using REPL as default .... is to make cling and REPL mutually exclusive. Which means we only use 1 of these by default.

  1. REPL by default , cling off by default
  2. And if cling is made on .... you can set(USE_CLING ON) and unset(USE_REPL) or anything related in cmake.

So that if we encounter something like (the case you've not addressed yet)

CppInterOp/.github/workflows/ci.yml

Lines 873 to 874 in f794dee

-DUSE_CLING=ON \
-DUSE_REPL=OFF \

We can just use USE_CLING and we don't need to explicitly set REPL to off.

Copy link
Collaborator

@anutosh491 anutosh491 Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

So we just end up with either the default case (which is supplying nothing, understood that repl is on and cling is off) or we just use cling=ON (understood repl would be off)

And if both if on by some chance we have the fatal error message.


if (USE_CLING AND USE_REPL)
message(FATAL_ERROR "We can only use Cling (USE_CLING=On) or Repl (USE_REPL=On), but not both of them.")
Expand Down
2 changes: 1 addition & 1 deletion lib/Interpreter/Compatibility.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -183,7 +183,7 @@ inline void codeComplete(std::vector<std::string>& Results,

#endif // USE_CLING

#ifdef USE_REPL
#ifndef USE_CLING

#include "DynamicLibraryManager.h"
#include "clang/AST/Mangle.h"
Expand Down
12 changes: 7 additions & 5 deletions unittests/CppInterOp/InterpreterTest.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
#include "cling/Interpreter/Interpreter.h"
#endif // USE_CLING

#ifdef USE_REPL
#ifndef USE_CLING
#include "clang/Interpreter/Interpreter.h"
#endif // USE_REPL

Expand Down Expand Up @@ -111,7 +111,8 @@ TEST(InterpreterTest, CreateInterpreter) {
EXPECT_TRUE(Cpp::GetNamed("cpp17"));
EXPECT_FALSE(Cpp::GetNamed("cppUnknown"));

#ifdef USE_REPL

#ifndef USE_CLING
// C API
auto CXI = clang_createInterpreterFromRawPtr(I);
auto CLI = clang_Interpreter_getClangInterpreter(CXI);
Expand Down Expand Up @@ -190,7 +191,7 @@ TEST(InterpreterTest, ExternalInterpreterTest) {
if (llvm::sys::RunningOnValgrind())
GTEST_SKIP() << "XFAIL due to Valgrind report";

#ifdef USE_REPL
#ifndef USE_CLING
llvm::ExitOnError ExitOnErr;
clang::IncrementalCompilerBuilder CB;
CB.SetCompilerArgs({"-std=c++20"});
Expand All @@ -203,7 +204,7 @@ if (llvm::sys::RunningOnValgrind())
std::unique_ptr<clang::Interpreter> I =
ExitOnErr(clang::Interpreter::create(std::move(CI)));
auto ExtInterp = I.get();
#endif
#endif // USE_REPL

#ifdef USE_CLING
std::string MainExecutableName = sys::fs::getMainExecutable(nullptr, nullptr);
Expand All @@ -223,9 +224,10 @@ if (llvm::sys::RunningOnValgrind())
#endif
EXPECT_TRUE(Cpp::GetInterpreter()) << "External Interpreter not set";

#ifdef USE_REPL
#ifndef USE_CLING
I.release();
#endif

#ifdef USE_CLING
delete ExtInterp;
#endif
Expand Down