sammccall marked 9 inline comments as done. sammccall added inline comments.
================ Comment at: clang/unittests/AST/AttrTest.cpp:19 +TEST(Attr, Doc) { + EXPECT_THAT(Attr::getDocumentation(attr::Used), + testing::HasSubstr("The compiler must emit the definition even " ---------------- aaron.ballman wrote: > It seems this is failing the premerge CI -- I think you need a call to > `.str()` in here to convert the `StringRef` to a `std::string`? Thanks - no idea why this only breaks MSVC, but gmock is a subtle beast... ================ Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:4237 + static const llvm::StringRef AttrDoc[] = { + #define ATTR(NAME) AttrDoc_##NAME, + #include "clang/Basic/AttrList.inc" ---------------- aaron.ballman wrote: > kadircet wrote: > > i am not well-versed in tablegen, so sorry if i am being dense here, butt > > what happens to the attributes we skipped above (e.g. the ones without a > > `ASTNode` flag)? > > > > Maybe we should be emitting a `... AttrDoc_X[] = "";` for all attributes, > > no matter what? > The ones without an AST node don't get an `attr::Kind` enumeration generated > for them, so I believe this interface is safe. Right, all those extra strings would just tickle -Wunused :-( I'd love to have an API that can also be used for attrs with ASTNode=0 (many are documented!). However: - for the motivating use case (hover), we can only trigger when we find an AST node anyway (sadly) - the obvious alternative is AttributeCommonInfo::Kind, (i.e. ParsedAttr kind), which isn't actually better, just differently bad. e.g. there's one ParsedAttr kind for `interrupt`, but three target-specific `Attr` kinds, each of which is documented separately. For hover you want to get the right one, and ParsedAttr kind can't choose it. ================ Comment at: clang/utils/TableGen/ClangAttrEmitter.cpp:4231 + // Only look at the first documentation if there are several. + // (As of now, only one attribute has multiple documentation entries). + break; ---------------- kadircet wrote: > not sure if this comment will stay useful. I want a comment to avoid a chesterton's fence: - the motivation for doing something lazy is that this is really rare - it's sensible to revisit this if it stops being rare Reworded it to make this more explicit. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D107703/new/ https://reviews.llvm.org/D107703 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits