-
Notifications
You must be signed in to change notification settings - Fork 32
Implement detection of resource and include dirs. #204
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
static bool exec(const char* cmd, std::vector<std::string>& outputs) { | ||
#define DEBUG_TYPE "exec" | ||
|
||
std::array<char, 256> buffer; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 256 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
std::array<char, 256> buffer;
^
|
||
std::string DetectResourceDir(const char* ClangBinaryName /* = clang */) { | ||
std::string cmd = std::string(ClangBinaryName) + " -print-resource-dir"; | ||
std::vector<std::string> outs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'outs' is not initialized [cppcoreguidelines-init-variables]
std::vector<std::string> outs; | |
std::vector<std::string> outs = 0; |
lib/Interpreter/CppInterOp.cpp
Outdated
|
||
std::string detected_resource_dir = outs.back(); | ||
|
||
#define Stringify(s) Stringifyx(s) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: function-like macro 'Stringify' used; consider a 'constexpr' template function [cppcoreguidelines-macro-usage]
#define Stringify(s) Stringifyx(s)
^
lib/Interpreter/CppInterOp.cpp
Outdated
std::string detected_resource_dir = outs.back(); | ||
|
||
#define Stringify(s) Stringifyx(s) | ||
#define Stringifyx(...) #__VA_ARGS__ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variadic macro 'Stringifyx' used; consider using a 'constexpr' variadic template function [cppcoreguidelines-macro-usage]
#define Stringifyx(...) #__VA_ARGS__
^
cmd += CompilerName; | ||
cmd += " -xc++ -E -v /dev/null 2>&1 | sed -n -e '/^.include/,${' -e '/^ " | ||
"\\/.*/p' -e '}'"; | ||
std::vector<std::string> outs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'outs' is not initialized [cppcoreguidelines-init-variables]
std::vector<std::string> outs; | |
std::vector<std::string> outs = 0; |
@@ -1,5 +1,8 @@ | |||
#include "clang/Interpreter/CppInterOp.h" | |||
|
|||
#include "llvm/ADT/SmallString.h" | |||
#include "llvm/Support/Path.h" | |||
|
|||
#include <gmock/gmock.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 'gmock/gmock.h' file not found [clang-diagnostic-error]
#include <gmock/gmock.h>
^
#endif // LLVM_BINARY_DIR | ||
Cpp::CreateInterpreter(); | ||
EXPECT_STRNE(Cpp::DetectResourceDir().c_str(), Cpp::GetResourceDir()); | ||
llvm::SmallString<256> Clang(LLVM_BINARY_DIR); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: 256 is a magic number; consider replacing it with a named constant [readability-magic-numbers]
llvm::SmallString<256> Clang(LLVM_BINARY_DIR);
^
} | ||
|
||
TEST(InterpreterTest, DetectSystemCompilerIncludePaths) { | ||
std::vector<std::string> includes; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: variable 'includes' is not initialized [cppcoreguidelines-init-variables]
std::vector<std::string> includes; | |
std::vector<std::string> includes = 0; |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #204 +/- ##
==========================================
+ Coverage 78.63% 78.74% +0.10%
==========================================
Files 8 8
Lines 3057 3091 +34
==========================================
+ Hits 2404 2434 +30
- Misses 653 657 +4
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
clang-tidy made some suggestions
lib/Interpreter/CppInterOp.cpp
Outdated
@@ -33,11 +33,20 @@ | |||
#include <sstream> | |||
#include <string> | |||
|
|||
#include <stdio.h> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
warning: inclusion of deprecated C++ header 'stdio.h'; consider using 'cstdio' instead [modernize-deprecated-headers]
#include <stdio.h> | |
#include <cstdio> |
eaea5ad
to
330edc2
Compare
This is useful when deployed CppInterOp needs to create an interpreter. Often the interpreter itself cannot use the clang toolchain logic to discover both the resource-dir and the include paths. This is because CppInterOp is a library and usually put in the `lib` folder whereas the toolchain logic expects things to be relative to the `bin` folder. In such setups, we can ask CppInterOp to detect the resource directory by asking clang about it. In the same vain, of finding C and C++ headers we can ask the system compiler about the include paths.
This is useful when deployed CppInterOp needs to create an interpreter. Often the interpreter itself cannot use the clang toolchain logic to discover both the resource-dir and the include paths. This is because CppInterOp is a library and usually put in the
lib
folder whereas the toolchain logic expects things to be relative to thebin
folder.In such setups, we can ask CppInterOp to detect the resource directory by asking clang about it. In the same vain, of finding C and C++ headers we can ask the system compiler about the include paths.