[PATCH] D148381: [WIP][Clang] Add counted_by attribute

[Bill Wendling via Phabricator via cfe-commits]

void marked 4 inline comments as done.
void added a comment.

In D148381#4625462 <https://reviews.llvm.org/D148381#4625462>, @nickdesaulniers wrote:

> I assume you plan to add some clang CodeGen tests at some point?

Yes. :-)



================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:531-532
 
-  /// True if CodeGen currently emits code implementing sanitizer checks.
-  bool IsSanitizerScope = false;
+  /// Non-zero if CodeGen currently emits code implementing sanitizer checks.
+  unsigned IsSanitizerScope = 0;
 
----------------
nickdesaulniers wrote:
> I don't understand this change. Grepping for `IsSanitizerScope` doesn't show any changed references to how this variable is used. Intentional?
Ah! Thanks for catching this. I had trouble with this scoping mechanism and for a while used a reference counting method to get around it, but put it back to its previous impl, except for this. I still think the `SanitizerScope` needs work, but that's beyond this project.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:17911
+        return FD;
+    }
+
----------------
nickdesaulniers wrote:
> unnecessary curly braces?
I prefer it this way in this case (actually, I wish C++ had a way to combine those two `if`'s into one). But it's not a big deal.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:17931
+  auto It = llvm::find_if(RD->fields(), [&](const FieldDecl *Field) {
+    return Field->getName() == ECA->getCountedByField()->getName();
+  });
----------------
rapidsna wrote:
> What happens in this corner case where the `Field` is actually the field that the attribute appertains to?
> ```
> struct foo {
>   int count;
>   int counted[__attribute__((counted_by(counted)))];
> };
> 
> ```
That should be an error. I'll add that as a check.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D148381