-
Notifications
You must be signed in to change notification settings - Fork 30
Remove unneeded MakeResourceDir #700
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
Remove unneeded MakeResourceDir #700
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #700 +/- ##
==========================================
- Coverage 79.74% 79.73% -0.02%
==========================================
Files 9 9
Lines 3955 3953 -2
==========================================
- Hits 3154 3152 -2
Misses 801 801
🚀 New features to boost your workflow:
|
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
e6523cb
to
4d5809c
Compare
clang-tidy review says "All clean, LGTM! 👍" |
This PR is ready for review. |
clang-tidy review says "All clean, LGTM! 👍" |
clang-tidy review says "All clean, LGTM! 👍" |
@@ -393,13 +393,6 @@ class SynthesizingCodeRAII { | |||
|
|||
namespace compat { | |||
|
|||
inline std::string MakeResourceDir(llvm::StringRef 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.
An alternative to removing MakeResourceDir like I've done here would be to move it to a different header file. It doesn't really belong in compatibility.h anymore since its not dependent on llvm version or operating system. @vgvassilev Is it ok just removing it, or would you prefer it be moved somewhere else? If its the latter, which header file do you suggest it gets moved to.
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.
No, this lgtm!
Description
Please include a summary of changes, motivation and context for this PR.
MakeResourceDir is in the compatibility header file despite the fact it is not dependent on what llvm version you are using, or whether you are using cling or clang-repl. Therefore this PR removes is, and just replaces the function in the code.
Fixes # (issue)
Type of change
Please tick all options which are relevant.
Testing
Please describe the test(s) that you added and ran to verify your changes.
Checklist