Closed cflaviu closed 1 year ago
Welcome!
How can I run the sanitizers locally, on my PC?
How can I run the sanitizers locally, on my PC?
I'm using either those delivered with the Ubuntu GCC (installable via apt-get
) or for the fuzzing part, I use the packages from https://apt.llvm.org/. On Mac there are downloads available here and on Windows I would probably use Ubuntu via WSL2.
I reviewed the code file by file and I made the changes ad hoc.
This doesn't mean you need to put them all into one commit. You can select the changes you want to stage for a commit and split the changes into several commits, each with an appropriate message to describe the changes. This will make the job of reviewing your changes easier. Even if you already committed, you can amend your commit or rewrite the git history before pushing.
The formatting is failing because I'm using clang-format 12 instead of 14.
I could made different commits but the pull requests will fail due clang-format. I have to install clang-format 14.
Thank you for your help!
Some points:
The
tuple.h
is actually generated, so the generator would need to be adapted to the style you introduced (using suffix "U" for unsigned, etc.): https://github.com/felixguendling/cista/blob/55f852feb04f0cd27d5742d649efd29fda8facea/tools/to_tuple_generator/to_tuple_generator.cc#L116Please use the formatter (clang-format with the contained
.clang-format
file).Regarding initialization of variables, I would prefer the explicit version:
bool self_allocated_{false};
instead ofbool self_allocated_{};
. It improves readability because it doesn't make me think "what is the default value for this data type"?It would be nice if you could make a separate commit for each type of change in the future to make it easier to review the changes:
const
/noexcept
32
->32U
)i != 0 || i != 1
->i > 1
)This time, it's fine.
For the
strong<T>
usage invector.h
I will make adjustments. This concept for strong typing of indices is not well-rounded yet. I'm working on it still. But maybe it's better to remove it for now from themaster
branch.