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

Reply via email to