sammccall added inline comments.

================
Comment at: clang-tools-extra/clangd/AST.cpp:434
+    for (AttributedTypeLoc ATL = *ATLPtr; !ATL.isNull();
+         ATL = ATL.getNextTypeLoc().getAs<AttributedTypeLoc>())
+      Result.push_back(ATL.getAttr());
----------------
hokein wrote:
> this looks like not safe, `getNextTypeLoc()` may return a null TypeLoc.
I don't think it can - an AttributedType always modifies a real type.
I've added an assert.


================
Comment at: clang-tools-extra/clangd/unittests/ASTTests.cpp:233
+  };
+  ASSERT_THAT(DeclAttrs("X"), Each(implicitAttr()));
+  ASSERT_THAT(DeclAttrs("Y"), Contains(attrKind(attr::WarnUnusedResult)));
----------------
hokein wrote:
> sorry, I'm not familiar with attributes, what is an implicit attr? It is 
> unclear to me why there is an attr for `class X`, the source code doesn't 
> have any attribute label for X (the same question for f and a)
Right, implicit attributes are when there's nothing written in the source ,but 
something else modifies the semantics in a way that clang authors decided to 
model as an attribute (e.g. because semantics match that of an explicit 
attribute).

I'm not familiar with many examples either, but a couple:
- when targeting windows, top-level classes appear to have an implicit "type 
visibility" attribute that I guess models the difference between default 
unix/windows symbol visibility.
- Aaron Ballman gave an example of `[[interrupt(...)]]` which also adds an 
implicit `[[used]]` attribute.

The windows example was why I assert there are no explicit attributes, instead 
of that there are none at all. I've added a comment.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D89785

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to