4 ms·

I'd swap 2 and 3. Getting CI, linting, auto-formatting, etc. going is a higher priority than tearing things out. Why? Because you don't know what to tear out ye

I'd swap 2 and 3. Getting CI, linting, auto-formatting, etc. going is a higher priority than tearing things out. Why? Because you don't know what to tear out yet or even the consequence of tearing them out. Linting (and other static analysis tools) also give you a lot of insight into where the program needs work. Things that get flagged by a static analysis tool (today) will often be areas where you can tear out entire functions and maybe even classes and files because they'll be a re-creation of STL concepts. Like homegrown iterator libraries (with subtle problems) that can be replaced with the STL algorithms library, or homegrown smart pointers that can just be replaced with actual smart pointers, or replacing the C string functions with C++'s on string class (and related classes) and functions/methods. But you won't see that as easily until you start scanning the code. And you won't be able to evaluate the consequences until you push towards more rapid test builds (at least) if not deployment.

broken_broken_ 3y ago
Fair point!
dralley 3y ago
On the flip side, auto-formatting will trash your version history and impede analysis of "when and why was this line added".
- skrebbel 3y ago
  You can ignore commits from git blame by adding them to a .gitattributes file. This is assuming Git of course, which is not a given at all for the average legacy c++ codebase.
  - fransje26 3y ago
    Good to know. Thanks for the tip!
- Jtsummers 3y ago
  I'm not hardcore on auto-formatters, but I think their impact on code history is negligible in the case of every legacy system I've worked on. The code history just isn't there. These aren't projects that used git until recently (if at all). Before that they used something else, but when they transitioned they didn't preserve the history. And that's if they used any version control system. I've tried to help teams whose idea of version control was emailing someone (they termed them "QA/CM") to make a read-only backup of the source directory every few months (usually at a critical review period in the project, so a lot of code was changed between these snapshots). That said, sure, skip them if you're worried about the history getting messed up or use them more selectively.
  - KerrAvon 3y ago
    SVN was a thing by the mid-2000's, and history from that is easy to preserve in git. Just how old are the sourcebases in question? (Not to shoot the messenger; just like, wow.) edit:typo
    - varjag 3y ago
      The first large C++ project I worked on in mid-1990s was basically preserving a bunch of archived copies of the source tree. CVS was a thing but not on Windows, and SourceSafe was creating more problems than it been solving.
      - mst 3y ago
        I kept regular tarballs of a project that used SourceSafe right near the start of my career, and found I was more likely to be able to find an intact copy of the right thing to diff against from my tarballs. I think after a year or so I realised that even bothering to -try- to use SourceSafe was largely silly, got permission to stop, and installed a CVS server on a dev box for my own use. (yes I know the VCS server shouldn't really be on the dev box I could potentially trash, I didn't have another machine handy and it was still a vast improvement)
    - Pfiffer 3y ago
- PreachSoup 3y ago
  On per file level it's just 1 commit. It's not really a big deal
- exDM69 3y ago
  clang-format can be applied to new changes only, for this very reason. Adding it will remove white space nitpicking from code review, even if it isn't perfect.
- IshKebab 3y ago
  I believe you can configure `git blame` to skip a specific commit. But in my experience it doesn't matter anyway for two reasons: 1. You're going to reformat it eventually anyway. You're just delaying things. The best time to plant a tree, etc. 2. If it's an old codebase and you're trying to understand some bit of code you're almost always going to have to walk through about 5 commits to get to the original one anyway. One extra formatting commit doesn't really make any difference.
- lpapez 3y ago
  You can instruct git to ignore specific commits for blame and diff commands. See "git blame ignore revs file". Intended use is exactly to ignore bulk changes like auto formatting.
  - westurner 3y ago
    +1 man git-blame git help blame https://git-scm.com/docs/git-blame https://git-scm.com/docs/git-blame
- duped 3y ago
  This is another reason why you should track important information in comments alongside the code instead of trusting VCS to preserve it in logs/commit messages, and to reject weird code missing comments from being merged. Not saying that fixes decades of cruft because you shouldn't change files without good reason and non-white space formatting is not a good reason, but I'm mentioning it because I've seen people naively belief bullshit like "code is self explanatory" and "the reason is in the commit message" Just comment your code folks, this becomes less of a problem
- mb7733 3y ago
  How does reformatting trash the history? It's one extra commit.. I guess if it splits or combines lines that could cause some noise if you really want the history of a single line... But that happens all the time, and I don't see how it would really prevent understanding the history. You can always do a blame on a range of lines. Maybe I'm missing something though, genuinely curious for a concrete example where reformatting makes it hard to understand history!
  - samus 3y ago
    If you ask the IDE to show blame info next to each line, then a lot of lines will be from the big reformatting. If course you can dig in and retrieve the history still, but it's an extras step then. Btw, it seems that at least Git has a way to make `git blame` avoid considering certain commits (.Git attributes). Maybe that works in IDEs too!
politician 3y ago
Nit: The post scopes "tearing things out" to dead code as guided by compiler warnings and unsupported architectures. If going the route, I'd recommend commenting out the lines rather than removing them outright to simplify the diffs at least until you're ready to squash and merge the branch.
- SAI_Peregrinus 3y ago
  Better to use `#if` or `#ifdef` to prevent compilation. C & C++ don't support nested comments, so you can end up with existing comments in the code ending the comment block.
  - kccqzy 3y ago
    I think `#if` and `#ifdef` are not good ideas because they prevent the compiler from seeing them in the first place. A better solution is just `if (false)` which is nestable, and the code is still checked by the compiler so it won't bit rot.
btown 3y ago
CI is different from the others, here! At minimum, building a "happy path(s)" test harness that can run with replicable results, and will run on every one of your commits, is a first step, and also helps to understand the codebase. And you're jumping around - and you'll have to! - odds are you'll have a bunch of things changed locally, and might accidentally create a commit that doesn't separate out one concern from another. CI will be a godsend at that point.
jasonwatkinspdx 3y ago
Yeah, I've done a fair bit of agency work dropping in to rescue code bases, and the first thing I do is run unit tests and check coverage. I add basic smoke tests anywhere they're missing. This actually speeds me up, rather than slowing me down, because once I have reasonably good coverage I can move dramatically faster when refactoring. It's a small investment that pays off.
thrwyexecbrain 3y ago
I would absolutely not recommend auto-formatting a legacy codebase. In my experience large C++ projects tend to have not only code generation scripts (python/perl/whatever) but also scripts that parse the code (usually to gather data for code generation). Auto formatting might break that. I have even seen some really cursed projects where the _users_ parsed public header files with rather fragile scripts.
- Jtsummers 3y ago
  I was listing the items in the original article's #3 and saying I'd move them up to #2 before I'd go about excising portions of the project, the original #2. I still stand by that. But you can read my other comment where I don't really defend auto-formatting to see that I don't care either way. I made it about four hours ago so maybe you missed it if you didn't refresh the page in the last few hours.

broken_broken_ 3y ago

Fair point!

dralley 3y ago

On the flip side, auto-formatting will trash your version history and impede analysis of "when and why was this line added".

skrebbel 3y ago
You can ignore commits from git blame by adding them to a .gitattributes file. This is assuming Git of course, which is not a given at all for the average legacy c++ codebase.
- fransje26 3y ago
  Good to know. Thanks for the tip!
Jtsummers 3y ago
I'm not hardcore on auto-formatters, but I think their impact on code history is negligible in the case of every legacy system I've worked on. The code history just isn't there. These aren't projects that used git until recently (if at all). Before that they used something else, but when they transitioned they didn't preserve the history. And that's if they used any version control system. I've tried to help teams whose idea of version control was emailing someone (they termed them "QA/CM") to make a read-only backup of the source directory every few months (usually at a critical review period in the project, so a lot of code was changed between these snapshots). That said, sure, skip them if you're worried about the history getting messed up or use them more selectively.
- KerrAvon 3y ago
  SVN was a thing by the mid-2000's, and history from that is easy to preserve in git. Just how old are the sourcebases in question? (Not to shoot the messenger; just like, wow.) edit:typo
  - varjag 3y ago
    The first large C++ project I worked on in mid-1990s was basically preserving a bunch of archived copies of the source tree. CVS was a thing but not on Windows, and SourceSafe was creating more problems than it been solving.
    - mst 3y ago
      I kept regular tarballs of a project that used SourceSafe right near the start of my career, and found I was more likely to be able to find an intact copy of the right thing to diff against from my tarballs. I think after a year or so I realised that even bothering to -try- to use SourceSafe was largely silly, got permission to stop, and installed a CVS server on a dev box for my own use. (yes I know the VCS server shouldn't really be on the dev box I could potentially trash, I didn't have another machine handy and it was still a vast improvement)
  - Pfiffer 3y ago
PreachSoup 3y ago
On per file level it's just 1 commit. It's not really a big deal
exDM69 3y ago
clang-format can be applied to new changes only, for this very reason. Adding it will remove white space nitpicking from code review, even if it isn't perfect.
IshKebab 3y ago
I believe you can configure `git blame` to skip a specific commit. But in my experience it doesn't matter anyway for two reasons: 1. You're going to reformat it eventually anyway. You're just delaying things. The best time to plant a tree, etc. 2. If it's an old codebase and you're trying to understand some bit of code you're almost always going to have to walk through about 5 commits to get to the original one anyway. One extra formatting commit doesn't really make any difference.
lpapez 3y ago
You can instruct git to ignore specific commits for blame and diff commands. See "git blame ignore revs file". Intended use is exactly to ignore bulk changes like auto formatting.
- westurner 3y ago
  +1 man git-blame git help blame https://git-scm.com/docs/git-blame https://git-scm.com/docs/git-blame
duped 3y ago
This is another reason why you should track important information in comments alongside the code instead of trusting VCS to preserve it in logs/commit messages, and to reject weird code missing comments from being merged. Not saying that fixes decades of cruft because you shouldn't change files without good reason and non-white space formatting is not a good reason, but I'm mentioning it because I've seen people naively belief bullshit like "code is self explanatory" and "the reason is in the commit message" Just comment your code folks, this becomes less of a problem
mb7733 3y ago
How does reformatting trash the history? It's one extra commit.. I guess if it splits or combines lines that could cause some noise if you really want the history of a single line... But that happens all the time, and I don't see how it would really prevent understanding the history. You can always do a blame on a range of lines. Maybe I'm missing something though, genuinely curious for a concrete example where reformatting makes it hard to understand history!
- samus 3y ago
  If you ask the IDE to show blame info next to each line, then a lot of lines will be from the big reformatting. If course you can dig in and retrieve the history still, but it's an extras step then. Btw, it seems that at least Git has a way to make `git blame` avoid considering certain commits (.Git attributes). Maybe that works in IDEs too!

politician 3y ago

Nit: The post scopes "tearing things out" to dead code as guided by compiler warnings and unsupported architectures. If going the route, I'd recommend commenting out the lines rather than removing them outright to simplify the diffs at least until you're ready to squash and merge the branch.

SAI_Peregrinus 3y ago
Better to use `#if` or `#ifdef` to prevent compilation. C & C++ don't support nested comments, so you can end up with existing comments in the code ending the comment block.
- kccqzy 3y ago
  I think `#if` and `#ifdef` are not good ideas because they prevent the compiler from seeing them in the first place. A better solution is just `if (false)` which is nestable, and the code is still checked by the compiler so it won't bit rot.

btown 3y ago

CI is different from the others, here! At minimum, building a "happy path(s)" test harness that can run with replicable results, and will run on every one of your commits, is a first step, and also helps to understand the codebase. And you're jumping around - and you'll have to! - odds are you'll have a bunch of things changed locally, and might accidentally create a commit that doesn't separate out one concern from another. CI will be a godsend at that point.

jasonwatkinspdx 3y ago

Yeah, I've done a fair bit of agency work dropping in to rescue code bases, and the first thing I do is run unit tests and check coverage. I add basic smoke tests anywhere they're missing. This actually speeds me up, rather than slowing me down, because once I have reasonably good coverage I can move dramatically faster when refactoring. It's a small investment that pays off.

thrwyexecbrain 3y ago

I would absolutely not recommend auto-formatting a legacy codebase. In my experience large C++ projects tend to have not only code generation scripts (python/perl/whatever) but also scripts that parse the code (usually to gather data for code generation). Auto formatting might break that. I have even seen some really cursed projects where the _users_ parsed public header files with rather fragile scripts.

Jtsummers 3y ago
I was listing the items in the original article's #3 and saying I'd move them up to #2 before I'd go about excising portions of the project, the original #2. I still stand by that. But you can read my other comment where I don't really defend auto-formatting to see that I don't care either way. I made it about four hours ago so maybe you missed it if you didn't refresh the page in the last few hours.