[CI] Improve Clang-Tidy and kill std::binds#1094
Conversation
Skylion007
requested review from
bigbike,
erikwijmans,
jturner65,
mosra and
vauduong
facebook-github-bot
added
the
CLA Signed
| -clang-analyzer-cplusplus.PlacementNew, | ||
| -modernize-avoid-bind, | ||
| ' # TODO fix std::binds later | ||
| # TODO Figure out if PlacementNew is a false positive |
There was a problem hiding this comment.
.../rapidjson/document.h:1143:21: error: Storage type is aligned to 1 bytes but allocated type is aligned to 8 bytes [clang-analyzer-cplusplus.PlacementNew,-warnings-as-errors]
Sigh. Not a false positive, no. Together with the RapidJson assert that prevents us from reliably running AddressSanitzer I'm starting to feel that we should drop RapidJson and replace it with something that is less of a dumpster fire...
| int nVertex = 0; | ||
| iss >> token >> nVertex; |
There was a problem hiding this comment.
... wait, why a custom PLY parser is still used here? 👀
There was a problem hiding this comment.
That's used just for processing the mp3d semantic ply's into what habitat ends up loading. IIRC we left this here because it's used just for that and the mp3d semantic ply's have a whole bunch of weird face attributes instead of our 1 weird one.
Skylion007
force-pushed
the
fix_sensor_memleak
branch
from
22e092f to
1e909f2
Compare
Skylion007
force-pushed
the
fix_sensor_memleak
branch
from
1e909f2 to
b44012e
Compare
| Mn::Vector2 nearPlaneSize_ = | ||
| Mn::Vector2{1.0f, static_cast<float>(height_) / width_}; | ||
| float scale; | ||
| float scale = NAN; |
There was a problem hiding this comment.
NAN means not-a-number.
And scale will be used later in nearPlaneSize_ /= scale;
I think scale = 1.0f would be a reasonable initial value.
There was a problem hiding this comment.
I would prefer NaN, as scale SHOULD be set. The reason to initially set it as NaN is to know something has gone horribly wrong in the constructor or elsewhere.
Scale will only be user later in nearPlaneSize_ /= scale after being parsed from the config value.
Skylion007
force-pushed
the
fix_sensor_memleak
branch
from
363c5c0 to
f7543b5
Compare
Skylion007
changed the title
| jsonConfig, "position", [lightAttribs](auto&& PH1) { | ||
| lightAttribs->setPosition(std::forward<decltype(PH1)>(PH1)); | ||
| }); |
There was a problem hiding this comment.
This is great! Thanks Aaron. I'm going through these and will rewrite the lamdas to be a bit more semantically meaningful.
6 participants
Motivation and Context
How Has This Been Tested
Types of changes
Checklist