search

sierrafoxtrot / srecord

SRecord github Mirror
https://srecord.sourceforge.net/
GNU General Public License v3.0
46 stars 23 forks source link

Modernization/updating c++ pass 1 #43

Closed SeanAlling-DojoFive closed 1 year ago

SeanAlling-DojoFive commented 1 year ago

First pass addressing Issue #41.

Pushing branch to to get exposure on changes.

sierrafoxtrot commented 1 year ago

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.

SeanAlling-DojoFive commented 1 year ago

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: @.***>

sierrafoxtrot commented 1 year ago

No worries, take your time. I'll kick off when you think it's ready.

SeanAlling-DojoFive commented 1 year ago

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.

jtxa commented 1 year ago

MegaLinter found a trailing whitespace.

Can you please also update .clang-tidy and remove those disabled messages that you fixed. So the QA can check them in future.

FYI, here is a summary, why the clang-tidy configuration looks like that, at the moment.

sierrafoxtrot commented 1 year ago

@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.

sierrafoxtrot commented 1 year ago

Hey @SeanAlling-DojoFive , I'm getting through this a bit each day. Confident that I'll have it wrapped on the weekend.