- 
                Notifications
    You must be signed in to change notification settings 
- Fork 35
Cppyy bug #401
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
Cppyy bug #401
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
There were too many comments to post at once. Showing the first 10 out of 19. Check the log or trigger a new build to see more.
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
There were too many comments to post at once. Showing the first 10 out of 14. Check the log or trigger a new build to see more.
| Codecov ReportAll modified and coverable lines are covered by tests ✅ 
 Additional details and impacted files@@            Coverage Diff             @@
##             main     #401      +/-   ##
==========================================
+ Coverage   70.84%   70.87%   +0.03%     
==========================================
  Files           9        9              
  Lines        3529     3533       +4     
==========================================
+ Hits         2500     2504       +4     
  Misses       1029     1029              
 
 | 
| @Vipul-Cariappa All tests pass locally, though I can see a bunch of clang - tidy suggestions. Can you have a look ? | 
That needs to be fixed at CppInterOp reference: look at comments at compiler-research/cppyy-backend#116
d218666    to
    18562fa      
    Compare
  
    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
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.
Looks good to me.
Enables test03_stllike_preinc at cppyy.
We should consider "squash and merge" and update the comment message to something more meaningful.
I might be a contributor for the poor initial commit message.
@vgvassilev if you can also approve this. I will go ahead and merge.
        
          
                lib/Interpreter/CppInterOp.cpp
              
                Outdated
          
        
      | { | ||
| auto D = (Decl *) var; | ||
| TCppType_t GetVariableType(TCppScope_t var) { | ||
| auto D = (Decl*)var; | 
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.
You should be able to satisfy clang-tidy with this
| auto D = (Decl*)var; | |
| auto *D = static_cast<Decl*>(var); | 
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
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 if the bots are happy.
`test03_stllike_preinc` fixed by compiler-research/CppInterOp#401 others fixed by compiler-research/CppInterOp#394
`test03_stllike_preinc` fixed by compiler-research/CppInterOp#401 others fixed by compiler-research/CppInterOp#394
Description
Please include a summary of changes, motivation and context for this PR.
Fixes #365
Type of change
Please tick all options which are relevant.
Checklist