[PATCH] D121873: [clang][extract-api] Add enum support

Thu Apr 28 12:32:55 PDT 2022

dexonsmith added inline comments.

================
Comment at: clang/test/ExtractAPI/enum.c:5
+// RUN: %t/reference.output.json
+// RUN: %clang -extract-api -target arm64-apple-macosx \
+// RUN: %t/input.h -o %t/output.json | FileCheck -allow-empty %s
----------------
I just noticed these tests use `%clang` instead of `%clang_cc1`. Usually it's best to test the driver in `clang/test/Driver` using `-###` (does the driver produce the right `-cc1` flags?) and then the compilation in isolation here using `%clang_cc1` and a `-cc1` command-line. Is it possible to update the tests to do that?

================
Comment at: clang/test/ExtractAPI/enum.c:6
+// RUN: %clang -extract-api -target arm64-apple-macosx \
+// RUN: %t/input.h -o %t/output.json | FileCheck -allow-empty %s
+
----------------
I think this will *always* be empty, even if there are warnings or errors, since those get written to `stderr` (not `stdout`). You could fix that by putting `2>&1` before the pipe, but see below:

================
Comment at: clang/test/ExtractAPI/enum.c:13-14
+
+// CHECK-NOT: error:
+// CHECK-NOT: warning:
+
----------------
It's usually much easier to test diagnostics using clangs builtin `-verify` flag instead of FileCheck. `-verify` is documented in the header: https://github.com/llvm/llvm-project/blob/main/clang/include/clang/Frontend/VerifyDiagnosticConsumer.h (maybe elsewhere too?).

This gives you good diffs between expected and unexpected diagnostics. In this case, you could replace these `CHECK-NOT`s with:
```
// expected-no-diagnostics
```
Usually you'd put that after the `RUN` line. When diagnostics are expected, you usually put those comments on the same line where you expect the diagnostic (there are tricks to avoid long lines and to handle other locations; see the docs).

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D121873