[PATCH] D24131: Add NewAddressDescription, which can describe any type of address.

[Vitaly Buka via llvm-commits]

vitalybuka accepted this revision.
vitalybuka added a comment.
This revision is now accepted and ready to land.

LGTM with few comments

In https://reviews.llvm.org/D24131#538393, @filcab wrote:

> Vitaly: I changed the patch a bit from yours. I think it ends up being simpler to have code like:
>
>   if (auto shadow = descr.AsShadow()) {
>     ...
>   } else ...
>   


LGTM

> Than to have the switch on .Kind(), since there's more code (reading) overhead with the switch.

> 

> I kept the const you added to the methods like Print(), but I can take them out and do a follow-up patch (or do it before) if you'd prefer.


Thanks


================
Comment at: lib/asan/asan_descriptions.h:198
@@ +197,3 @@
+                     bool shouldLockThreadRegistry = true) {
+    if (GetShadowAddressInformation(addr, &data.shadow)) {
+      data.kind = kAddressKindShadow;
----------------
Could you please hide this constructor into cpp?
also GetHeapAddressInformation and DescribeAddressIfStack can be removed from h.

================
Comment at: lib/asan/asan_descriptions.h:227
@@ +226,3 @@
+
+  // AddressKind Kind() const { return data.kind; }
+  uptr Address() const {
----------------
Please remove commented line


https://reviews.llvm.org/D24131