-
Notifications
You must be signed in to change notification settings - Fork 23
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
Modernization/updating c++ pass 1 #43
Modernization/updating c++ pass 1 #43
Conversation
bf343d7
to
57c3e51
Compare
Thanks @SeanAlling-DojoFive, this is looking great. I'm hoping to get through the review today. If there are no questions/issues, are you happy if I squash-merge? I'm still getting the lay of the land on what is considered good form on GH for merging contributions and to me this one looks like a hefty but single update if that makes sense. |
That is fine, I am still looking at some cleanup and verifying this are all correct.
Should have my changes done this weekend and then I’ll do a squash merge.
…On Jan 28, 2023 at 15:03 -0800, Scott Finneran ***@***.***>, wrote:
Thanks @SeanAlling-DojoFive, this is looking great. I'm hoping to get through the review today.
If there are no questions/issues, are you happy if I squash-merge? I'm still getting the lay of the land on what is considered good form on GH for merging contributions and to me this one looks like a hefty but single update if that makes sense.
—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you were mentioned.Message ID: ***@***.***>
|
No worries, take your time. I'll kick off when you think it's ready. |
Okay, I have gone through every file and attempted to move all default member functions to be in class. @sierrafoxtrot I think now is a good time to start taking a look. My ultimate goal of this first PR is to address update all constructors. Other PR's will focus on other C++11 modernization's. |
a838629
to
d51468d
Compare
6168fc6
to
a9cf1bb
Compare
MegaLinter found a trailing whitespace. Can you please also update FYI, here is a summary, why the clang-tidy configuration looks like that, at the moment. |
a9cf1bb
to
173d205
Compare
@SeanAlling-DojoFive , it's might be worth noting that I need to manually approve the running of the checks which is no doubt a pain (I can relax this setting but not for recently created GH accounts :-( ). We likely have a timezone difference which is slowing things down for you. If you submit a separate smaller commit, I can merge and GH will no longer consider you a first-time-committer and the checks should trigger automatically speeding up the cycle. |
c4765d9
to
b04db29
Compare
Hey @SeanAlling-DojoFive , I'm getting through this a bit each day. Confident that I'll have it wrapped on the weekend. |
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.
Thank you Sean for this extensive contribution. My apologies for the time taken to respond but I wanted to do it justice. A couple of minor comments.
Four clang-tidy riles used 1. cppcoreguidelines-pro-type-member-init 1. modernize-use-default-member-init 1. readability-redundant-member-init 1. readability-redundant-string-init [modernization] updated existing constructors [modernization] Added missing delete statements [modernization] Resolved megalinter errors [modernization] Updated multiple constructors Pt.1 [modernization] Updated multiple constructors Pt.2
b04db29
to
cfc2f47
Compare
First pass addressing Issue #41.
Pushing branch to to get exposure on changes.