CSUS-LLVM / OptSched

Optimizing scheduler. Combinatorial instruction scheduling project.
GNU General Public License v3.0
19 stars 17 forks source link

Fix variable and function names #176

Open Quincunx271 opened 2 years ago

Quincunx271 commented 2 years ago

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 https://github.com/CSUS-LLVM/OptSched/issues/47#issuecomment-597386241

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

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

    Originally posted by @Quincunx271 in https://github.com/CSUS-LLVM/OptSched/issues/9#issuecomment-674622805

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 commented 2 years ago

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 commented 2 years ago

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

Quincunx271 commented 2 years ago

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.