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

Hide header files that are not part of the interface. #158

Merged
merged 1 commit into from
Oct 30, 2023

Conversation

vgvassilev
Copy link
Contributor

Header files that are only used in the implementation files can become local to the library. This would simplify the installation.

Header files that are only used in the implementation files can become local
to the library. This would simplify the installation.
@codecov
Copy link

codecov bot commented Oct 30, 2023

Codecov Report

Merging #158 (cd20749) into main (6069bb1) will decrease coverage by 0.05%.
The diff coverage is 88.57%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main     #158      +/-   ##
==========================================
- Coverage   57.26%   57.21%   -0.05%     
==========================================
  Files           8        8              
  Lines        2878     2877       -1     
==========================================
- Hits         1648     1646       -2     
- Misses       1230     1231       +1     
Files Coverage Δ
lib/Interpreter/CppInterOp.cpp 79.48% <ø> (ø)
lib/Interpreter/DynamicLibraryManager.cpp 55.73% <ø> (ø)
lib/Interpreter/DynamicLibraryManagerSymbol.cpp 0.00% <ø> (ø)
lib/Interpreter/Paths.cpp 25.09% <ø> (ø)
lib/Interpreter/DynamicLibraryManager.h 70.00% <70.00%> (ø)
lib/Interpreter/CppInterOpInterpreter.h 74.53% <89.65%> (ø)
lib/Interpreter/Compatibility.h 90.27% <90.27%> (ø)
Files Coverage Δ
lib/Interpreter/CppInterOp.cpp 79.48% <ø> (ø)
lib/Interpreter/DynamicLibraryManager.cpp 55.73% <ø> (ø)
lib/Interpreter/DynamicLibraryManagerSymbol.cpp 0.00% <ø> (ø)
lib/Interpreter/Paths.cpp 25.09% <ø> (ø)
lib/Interpreter/DynamicLibraryManager.h 70.00% <70.00%> (ø)
lib/Interpreter/CppInterOpInterpreter.h 74.53% <89.65%> (ø)
lib/Interpreter/Compatibility.h 90.27% <90.27%> (ø)

@vgvassilev vgvassilev merged commit cc70791 into compiler-research:main Oct 30, 2023
8 of 10 checks passed
@vgvassilev vgvassilev deleted the hide-headers branch October 30, 2023 08:42
Copy link
Contributor

@github-actions github-actions bot left a 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

There were too many comments to post at once. Showing the first 10 out of 29. Check the log or trigger a new build to see more.

#include "clang/AST/Mangle.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Interpreter/Interpreter.h"
#include "clang/Interpreter/Value.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'clang/Interpreter/Value.h' file not found [clang-diagnostic-error]

#include "clang/Interpreter/Value.h"
         ^

Comment on lines +6 to +7
#ifndef CPPINTEROP_COMPATIBILITY_H
#define CPPINTEROP_COMPATIBILITY_H
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: header guard does not follow preferred style [llvm-header-guard]

Suggested change
#ifndef CPPINTEROP_COMPATIBILITY_H
#define CPPINTEROP_COMPATIBILITY_H
#ifndef GITHUB_WORKSPACE_LIB_INTERPRETER_COMPATIBILITY_H
#define GITHUB_WORKSPACE_LIB_INTERPRETER_COMPATIBILITY_H

lib/Interpreter/Compatibility.h:310:

- #endif // CPPINTEROP_COMPATIBILITY_H
+ #endif // GITHUB_WORKSPACE_LIB_INTERPRETER_COMPATIBILITY_H

#include "clang/AST/Mangle.h"
#include "clang/Frontend/CompilerInstance.h"
#include "clang/Interpreter/Interpreter.h"
#include "clang/Interpreter/Value.h"
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: 'clang/Interpreter/Value.h' file not found [clang-diagnostic-error]

#include "clang/Interpreter/Value.h"
         ^

return Arg == match;
};
auto it = std::find_if(args.begin(), args.end(), has_arg);
std::vector<const char*> gpu_args = {it, args.end()};
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'gpu_args' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::vector<const char*> gpu_args = {it, args.end()};
std::vector<const char*> gpu_args = 0 = {it, args.end()};

clang::IncrementalCompilerBuilder CB;
CB.SetCompilerArgs({args.begin(), it});

std::unique_ptr<clang::CompilerInstance> DeviceCI;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'DeviceCI' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::unique_ptr<clang::CompilerInstance> DeviceCI;
std::unique_ptr<clang::CompilerInstance> DeviceCI = 0;

// copied and adapted from CodeGen::CodeGenModule::getMangledName

clang::NamedDecl* D =
llvm::cast<clang::NamedDecl>(const_cast<clang::Decl*>(GD.getDecl()));
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]

      llvm::cast<clang::NamedDecl>(const_cast<clang::Decl*>(GD.getDecl()));
                                   ^


clang::NamedDecl* D =
llvm::cast<clang::NamedDecl>(const_cast<clang::Decl*>(GD.getDecl()));
std::unique_ptr<clang::MangleContext> mangleCtx;
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: variable 'mangleCtx' is not initialized [cppcoreguidelines-init-variables]

Suggested change
std::unique_ptr<clang::MangleContext> mangleCtx;
std::unique_ptr<clang::MangleContext> mangleCtx = 0;

inline llvm::orc::LLJIT* getExecutionEngine(clang::Interpreter& I) {
#if CLANG_VERSION_MAJOR >= 14
auto* engine = &llvm::cantFail(I.getExecutionEngine());
return const_cast<llvm::orc::LLJIT*>(engine);
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: do not use const_cast [cppcoreguidelines-pro-type-const-cast]

  return const_cast<llvm::orc::LLJIT*>(engine);
         ^

Comment on lines +240 to +241
namespace Cpp_utils = Cpp::utils;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: namespace alias decl 'Cpp_utils' is unused [misc-unused-alias-decls]

Suggested change
namespace Cpp_utils = Cpp::utils;
}
}


// Clang >= 14 change type name to string (spaces formatting problem)
#if CLANG_VERSION_MAJOR >= 14
inline std::string FixTypeName(const std::string type_name) {
Copy link
Contributor

Choose a reason for hiding this comment

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

warning: the const qualified parameter 'type_name' is copied for each invocation; consider making it a reference [performance-unnecessary-value-param]

Suggested change
inline std::string FixTypeName(const std::string type_name) {
inline std::string FixTypeName(const std::string& type_name) {

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant