[PATCH] D57914: [Driver] Allow enum SanitizerOrdinal to represent more than 64 different sanitizer checks, NFC.

[Bruno Ricci via Phabricator via cfe-commits]

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