[PATCH] D155879: Summary: [asan][win][msvc] override new and delete and seperate TUs

[Nicole Mazzuca via Phabricator via llvm-commits]

strega-nil requested changes to this revision.
strega-nil added a comment.
This revision now requires changes to proceed.

Some minor nits, mostly.



================
Comment at: compiler-rt/lib/asan/asan_win_new_delete.cpp:27
+
+struct __asan_win_new_delete_data {
+  size_t size;        // Size of this struct (it travels over the DLL boundary).
----------------
Why is this defined differently between `asan_win_new_delete.cpp` and `asan_win_new_delete_thunk_common.h`?


================
Comment at: compiler-rt/lib/asan/asan_win_new_delete_thunk_common.h:42
+// clang-format off
+// Fallback Ordering for new/delete
+//
----------------
I know this is a nit, but 147 columns is super excessive and means I can't read this block comment on my laptop screen without it wrapping. Can these be split somehow?


================
Comment at: compiler-rt/lib/asan/asan_win_new_delete_thunk_common.h:72
+// Only need definition detection for overloads with children.
+enum defined_ops {
+  op_new_scalar,
----------------
Style nit: I don't like that this type is `snek_case`, and this should also likely be an enum class. Additionally, the enumerators are `snek_case` as well, as opposed to the two standard `SCREAMING_SNAKES` or `kCamelCase` from the rest of compiler-rt.


================
Comment at: compiler-rt/lib/asan/asan_win_new_delete_thunk_common.h:95
+
+  static int defined;
+};
----------------
I believe this could be `const`.

Additionally, I absolutely _hate_ this name; I would much prefer something like `static const bool is_asan;` or `static const bool has_user_overload;` (with reversed meaning).


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155879/new/

https://reviews.llvm.org/D155879