Simplify and clang tidy #460
Conversation
|
Thanks, I've cherry-picked most of this PR. Could you please rebase it so we can discuss the rest? There are a few things I'd like to see changed. The main of them is the clang-tidy configuration that causes it to put small functions and loops/conditionals entirely on one line (which IMO makes the code more annoying to read); ideally it'd not reformat the functions that way at all, and maybe match loops/conditionals to the existing style better, but the second part isn't as important. The rest are something I'd like to discuss in review. |
|
@rpavlik Also, looks like tests crash on Windows now. Could you please take a look? https://ci.appveyor.com/project/whitequark/solvespace/builds/27299022 |
|
Looks like the crash is caused by "Range-for conversions and improvements. NFC." |
|
Yeah, I have hung my machine twice now attempting to run the tests for that commit with sanitizers from GCC enabled. (It was all fine with Clang...) I've dropped that patch from this series, will figure it out separately. (Yes, I am now testing that each commit in a branch can be built and passes tests before pushing - embarassed myself too much recently with broken commits.) I have adjusted the clang-format file and re-formatted my changes here as well to reduce the number of one-line things that get created. |
|
@rpavlik This PR still fails on Windows. |
src/polygon.h
Outdated
| void Generate(std::function<void(Vertex *start, Vertex *next, Edge *edge)> const &startFunc, | ||
| std::function<void(Vertex *next, Edge *edge)> const &nextFunc, | ||
| std::function<void(Edge *)> const &aloneFunc, | ||
| std::function<void()> const &endFunc = []() {}); |
There was a problem hiding this comment.
What does this actually guarantee?
There was a problem hiding this comment.
the change to add const & there prevents copying of the functor object, which can be expensive. The requirement is that the Generate function shouldn't hold those references longer than its call (or longer than the lifetime of the thing being passed in, which is at least as long as the call).
I see I accidentally did east-const there instead of west-const. I'll take that as a TODO.
There was a problem hiding this comment.
Oh yeah, I got confused by std::function<> const, it should really be const std::function<>. I wasn't sure if the semantics is identical.
|
Merged a few more commits from here. In "Simplify returns and booleans. NFC.", most of the changes make the code less readable, and a few comments no longer explain what's happening, so you can drop that one. |
|
ok, guess I will reboot into Windows for this :) (the bug is in the const/const-refs commit - pulled that one from this branch to a separate one) |
|
Merged everything except FindIndex changes. Do we really have no better approach here than returning -1 if it's not found? Morally it seems like FindIndex ought to return size_t. |
|
FWIW, the |
|
Yeah that'd probably be fine. |
These are generic and work with List and IdList, as well as vector<> and raw arrays.
|
Rebased to leave the one commit not cherry-picked yet. |
phkahler
force-pushed
the
master
branch
2 times, most recently
from
dfb96de
to
3f09eaf
Compare
Take or leave any subset of these patches. They're all split by topic for easy cherry-picking.