Skip to content
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 variable and function names #176

Open
Quincunx271 opened this issue Oct 30, 2021 · 3 comments
Open

Fix variable and function names #176

Quincunx271 opened this issue Oct 30, 2021 · 3 comments
Labels
cleanup enhancement New feature or request

Comments

@Quincunx271
Copy link
Member

Breaking off from #47. There are two ideas at play here:

  1. We have a lot of shortened names which are hard to read:

    I would also like to suggest renaming of functions and variables. UpdtRdyLst_ is not much shorter than UpdateRdyList_ or UpdateReadyList_, but those are much easier to read. There's a pervasive removal of vowels: Dynmc, PrirtyLst, RcrsvScsr.

    Some shortenings are reasonable (Reg), but there should be very few of these. The pattern I've noticed is that I'm generally fine if it's a single syllable of the larger word, rather than the larger word with vowels removed: Reg vs Rgstr

    Originally posted by @Quincunx271 in General Refactoring Planning #47 (comment)

  2. Many of our names break the clang-tidy check set up by LLVM:

    To be clear on the state of this: Fix clang-tidy warnings #77 fixed the majority of these cases. What's left to fix is the casing of our variable names.

    Originally posted by @Quincunx271 in Fix Clang-Tidy violations? #9 (comment)

This is not an issue that can be fixed in one PR. It must be split into many. This is also significantly easier if we practice short branches rather than long-lived feature branches, else merge conflicts will be rampant and hard to fix.

Some advice on tackling this:

  1. For expanding shortened names:
    • If the expansion can be accomplished by a script, it can be worth committing said script to the repo to aide in merge conflict resolution.
  2. For fixing the remaining clang-tidy check:
    • The check needs to be re-enabled in some form. Currently, it's not very visible in the CI checks. Ideally, it would complain for new names which do not follow the rules.
@jrbyrnes
Copy link
Contributor

jrbyrnes commented Oct 30, 2021

I think Visual Studio allows for automated renaming (F2 on a symbol) -- this may only be with C/C++ (Microsoft) extension. I haven't tried this feature, so I can't speak on its reliability, however it could be useful here.

As a side note, earlier I defended the weird vowel-removed naming scheme, however I have grown to dislike it -- I agree with renaming.

@Quincunx271
Copy link
Member Author

If the F2 rename works (if it compiles afterwards), then that's probably the easiest technique.

@Quincunx271
Copy link
Member Author

Actually, a better idea might be to have two versions of the function for some period of time (e.g. two months) to enable people to merge the changes into their branch. That is, keep the old function name around as an alias for the new name so that the migration can be done non-atomically.

For example:

// Before:
void UpdtRdyLst_(...) { 
    // The code
}

// After, pretending this change happened 2021-11-07:
void updateReadyList_(...) {
    // The code
}

[[deprecated("Renamed to updateReadyList_. Will be removed after 2022-01-07")]]
void UpdtRdyLst_(...) {
    updateReadyList_(...);
}

This action could be used to schedule the function deletion by creating a branch with the deletion and PR-ing it with a scheduled merge: https://github.com/marketplace/actions/merge-schedule.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cleanup enhancement New feature or request
Projects
None yet
Development

No branches or pull requests

2 participants