Improve code readability. Exhaustive changes for each class of cleanup. #14178

pull practicalswift wants to merge 2 commits into bitcoin:master from practicalswift:september-cleanups changing 3 files +4 −3

practicalswift commented at 1:48 PM on September 9, 2018: contributor
Improve code readability:
- Use auto* instead of auto for pointers
- ~~Avoid shadowing variable~~
- Separate assignment from function call
- ~~Remove redundant code~~
These changes are all exhaustive.
readability: Use auto* instead of auto for pointers bce582d8ae
practicalswift renamed this:
~~Improve code readability~~
Improve code readability. Exhaustive changes for each cleanup.
on Sep 9, 2018
practicalswift renamed this:
~~Improve code readability. Exhaustive changes for each cleanup.~~
Improve code readability. Exhaustive changes for each class of cleanup.
on Sep 9, 2018
in src/qt/askpassphrasedialog.cpp:244 in eb85079f12 outdated
```
 243 | -    ui->passEdit2->setEchoMode(mode);
 244 | -    ui->passEdit3->setEchoMode(mode);
 245 | +    const auto echoMode = show ? QLineEdit::Normal : QLineEdit::Password;
 246 | +    ui->passEdit1->setEchoMode(echoMode);
 247 | +    ui->passEdit2->setEchoMode(echoMode);
 248 | +    ui->passEdit3->setEchoMode(echoMode);
```
MarcoFalke commented at 2:17 PM on September 9, 2018:

We don't do these changes, since it is non-trivial to prove the refactoring does the right thing: #10136

practicalswift commented at 3:05 PM on September 9, 2018:

Wasn't the problem that people used ancient compilers? That this only remaining case is correct should be obvious to us humans, no? :-)

This was one of the comments:

And there is still value in compiling with Wshadow periodically (with a carefully chosen compiler) and seeing if it finds any bug.

I'm using a carefully chosen compiler :-)
MarcoFalke commented at 3:25 PM on September 9, 2018:

this only remaining case

This clearly isn't the only remaining case.

See for example rest.cpp

auto process_utxos = [&vOutPoints, &outs, &hits](const CCoinsView& view, const CTxMemPool& mempool) {

where one of the args shadows the global mempool
practicalswift commented at 6:36 PM on September 9, 2018:

Oh, I misread a report. Sorry. Reverted the shadowing change :-)
MarcoFalke commented at 2:19 PM on September 9, 2018: member

Tend to NACK. All of these are subjective style cleanups. Unless they have a major impact on developer experience or user experience, they are probably not worth to "fix".
practicalswift force-pushed on Sep 9, 2018
practicalswift commented at 3:12 PM on September 9, 2018: contributor

@MarcoFalke I've removed what I see as the most "subjective" changes. Do you think that any of the remaining ones are acceptable or should I close? :-)

Hopefully separating assignment from function call should be fairly non-controversial? And using auto* instead of auto to show that a pointer is being used should be an obvious win for readability? :-)

Note that each change is exhaustive so these are the only cases.
readability: Separate assignment from function call f0171d0187

in src/qt/addressbookpage.cpp:41 in b9bc483a47 outdated

  37 | @@ -38,7 +38,7 @@ class AddressBookSortFilterProxyModel final : public QSortFilterProxyModel
  38 |  protected:
  39 |      bool filterAcceptsRow(int row, const QModelIndex& parent) const
  40 |      {
  41 | -        auto model = sourceModel();
  42 | +        auto* model = sourceModel();

MarcoFalke commented at 3:26 PM on September 9, 2018:

This change makes it harder to switch to smart pointers later, no?

practicalswift force-pushed on Sep 9, 2018
practicalswift commented at 11:18 AM on September 10, 2018: contributor

Closing I misread a report: these fixes are not exhaustive. Sorry.
practicalswift closed this on Sep 10, 2018
practicalswift deleted the branch on Apr 10, 2021
DrahtBot locked this on Aug 18, 2022

Contributors

practicalswift

MarcoFalke