riccibruno marked an inline comment as done.
riccibruno added a comment.
Looks mostly fine to me. I have added a few inline comments but they are all
little nits.
================
Comment at: include/clang/Basic/Sanitizers.h:39
+struct SanitizerMask {
+private:
+ // Number of array elements.
----------------
`struct` -> `class` and get rid of the `private` ?
================
Comment at: include/clang/Basic/Sanitizers.h:46
+ static constexpr unsigned kNumBits = sizeof(maskLoToHigh) * 8;
+ // Number of bits in a mask element.
+ static constexpr unsigned kNumBitElem = sizeof(maskLoToHigh[0]) * 8;
----------------
/// instead to have doxygen handle these comments (here and elsewhere).
================
Comment at: include/clang/Basic/Sanitizers.h:51
+ // Mask is initialized to 0.
+ SanitizerMask() : maskLoToHigh{} {}
+
----------------
You don't have to explicitly write the default ctor; you can just instead write
`uint64_t maskLoToHigh[kNumElem]{};` above to value-initialize each element in
the array.
================
Comment at: include/clang/Basic/Sanitizers.h:54
+ static constexpr bool
+ checkBitPos(const SanitizerKind::SanitizerOrdinal &Pos) {
+ return Pos < kNumBits;
----------------
`SanitizerOrdinal` should probably be passed by value here and elsewhere.
================
Comment at: include/clang/Basic/Sanitizers.h:61
+ bitPosToMask(const SanitizerKind::SanitizerOrdinal &Pos) {
+ assert(Pos < kNumBits);
+ SanitizerMask mask;
----------------
Could you please add a relevant message to the assertion (here and elsewhere) ?
================
Comment at: include/clang/Basic/Sanitizers.h:96
+ return false;
+ }
+ return true;
----------------
Is that vectorized by gcc/clang, or is that a sequence of branches ?
================
Comment at: lib/Driver/SanitizerArgs.cpp:53
+SanitizerMask CompatibleWithMinimalRuntime =
+ TrappingSupported | Scudo | ShadowCallStack;
----------------
Shouldn't all of these masks be const to be sure that they are not modified by
mistake (unless I a missing something and they are modified somewhere else) ?
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D57914/new/
https://reviews.llvm.org/D57914
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
