-
Notifications
You must be signed in to change notification settings - Fork 26
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
Fix finding Resource Dir #171
Fix finding Resource Dir #171
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #171 +/- ##
==========================================
+ Coverage 80.76% 80.78% +0.01%
==========================================
Files 19 19
Lines 972 973 +1
Branches 93 93
==========================================
+ Hits 785 786 +1
Misses 187 187
|
clang-tidy review says "All clean, LGTM! 👍" |
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.
There is nothing wrong with the current approach: https://godbolt.org/z/5GzTeYo93 I do not think this needs fixing. Consider closing this PR.
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.
I think I see what you mean, because we have an std::vector<const char*>
we can't really use the operator== of std::string. Apologies for the oversight.
Co-authored-by: Vassil Vassilev <[email protected]>
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
Co-authored-by: Vassil Vassilev <[email protected]>
clang-tidy review says "All clean, LGTM! 👍" |
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.
Lgtm!
Description
std::find
operates based on the operator==
of the type being compared. Forconst char*
, theoperator==
compares the memory addresses (the pointer values), not the contents of the strings.Hence although we should fetch resource dir from the kernel.json file, we end up entering the block checking for resource dir and trying getting it through CppInterOp. This should not be the case.
Type of change
Please tick all options which are relevant.