kadircet marked 13 inline comments as done. kadircet added inline comments.
================ Comment at: clang-tools-extra/include-cleaner/include/clang-include-cleaner/Analysis.h:14 +#include "llvm/ADT/STLFunctionalExtras.h" +#include <variant> + ---------------- sammccall wrote: > This is awful but: as mentioned on another patch, we can't actually use > variant. AFAIK all C++17 *language* features are available, but LLVM supports > apple clang 9.3, which doesn't provide <variant> at all. > > There's an IntrusiveVariant in review somewhere but seems stalled (and > honestly the Intrusive part makes it pretty ugly to use). > > I think for now our best option is a Kind enum and some kind of uintptr_t > storage :-( right, i've seen that comment as well and wanted to pile up there. there are already uses of `<variant>` in LLVM today, TableGen, DenseMap, CodeGen, flang, pseudo, MLIR and lldb are some examples of big components. I don't see what they're doing to compile those today. ================ Comment at: clang-tools-extra/include-cleaner/unittests/WalkASTTest.cpp:125 +TEST(WalkUsed, Basic) { + llvm::Annotations HeaderCode(R"cpp( ---------------- sammccall wrote: > signal-to-noise in this test is a bit low, and there'll be a bunch more tests > as the features get extended. > > I think adding a `TEST_F` fixture and splitting the stdlib out into its own > test, trying to maximize the shared code, may lead to cleaner tests. > > Feel free to defer this, just watch out for incremental inertia giving you a > 1000 line monster before you know it! yup, that's definitely something i've in pending patches. Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D136293/new/ https://reviews.llvm.org/D136293 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits