[PATCH] D20913: [asan] add primitives that allow coroutine implementations

[Filipe Cabecinhas via llvm-commits]

filcab added inline comments.

================
Comment at: lib/asan/asan_thread.cc:14
@@ -13,1 +13,3 @@
 //===----------------------------------------------------------------------===//
+#include <stddef.h>
+
----------------
blastrock wrote:
> filcab wrote:
> > Isn't this one of the files that shouldn't include system headers?
> I don't know about that. I use it for size_t to implement the functions exposed in common_interface_defs.h. I can move these implementations in another file if needed, or use uptr if that makes sense, but I'm not sure I can use it in common_interface_defs.
OK. Remove the `stddef.h` include.
On the external header, stddef.h is already included (it's external, so we don't have a problem with that) and you can keep `size_t` there.
Internally, use `uptr` instead of `size_t`. We know that on the platforms we support we're safe doing it.


================
Comment at: lib/asan/asan_thread.cc:165
@@ +164,3 @@
+    if (fiber_stack_bottom_)
+      return StackBounds{fiber_stack_bottom_, fiber_stack_top_}; // NOLINT
+    else
----------------
blastrock wrote:
> filcab wrote:
> > ugh, the linter warns here?
> It says that a semicolon after a closing brace is useless, it probably thinks of this as a block...
Unfortunate, but not much to do here.

================
Comment at: lib/asan/asan_thread.cc:176
@@ +175,3 @@
+      return StackBounds{next_fiber_stack_bottom_, // NOLINT
+                         next_fiber_stack_top_};  // NOLINT
+    else
----------------
blastrock wrote:
> filcab wrote:
> > Line up the `//NOLINT` unless clang-format changes it to be unaligned.
> > But given the different amount of spaces in these two lines, it seems you should at least run clang-format ;-)
> I think I did run clang-format, I will do it again. I just need to use the default llvm style, right?
You can probably use Google-style for ASan. I'm not sure if we're trying to transition ASan to LLVM-style, but I don't think so.


http://reviews.llvm.org/D20913