Add ExtractAPI support C++ classes, fields, methods, and various qualifiers and specifiers
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Starting to come together nicely, but I think now would be a good time to write some tests. I am particularly interested in more complex situations like inheritance hierarchies.
clang/include/clang/ExtractAPI/API.h | ||
---|---|---|
25 | Inste | |
637 | Not sure this is correct, the semantic relationship is that structs and unions can be classes. This expresses that all struct records and unions are CXXClassRecords. It would be best to keep these entirely separate as classof is used by the type casting functions, e.g. llvm::dynamic_cast and we certainly don't want arbitrary struct or union record to be cast-able to CXXClassRecord. | |
776 | mega nit: I suggest grouping together the has_function_signature trait and the has_access trait separately, i.e. moving the empty line from 770 to between 772 and 773 | |
clang/include/clang/ExtractAPI/DeclarationFragments.h | ||
185 | Should this be API.h? | |
196 | Instead of storing a reference to the string here, it might make sense to store the string itself, I am worried that for future usages in libclang the string references would go stale. | |
clang/include/clang/ExtractAPI/ExtractAPIVisitor.h | ||
319–320 | Is it to short circuit to only call VisitRecordDecl if so I think that it should call getDerivedExtractAPIVisitor().VisitRecordDecl | |
401 | Is there a way of only looking at public bases here? Non-public inheritance is not part of the API surface of the class. | |
clang/lib/ExtractAPI/API.cpp | ||
19 | AccessControl should move into API.h so we don't need to pull in DeclarationFragments.h | |
44 | This just pollutes the git history, would be best to remove this. | |
clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp | ||
607 | We still need all symbols to have an "accessLevel" specifier. Maybe the false_type overload of serializeAccessMixin could make "public" the default. |
Address some review feedback
Remove RK_Struct, RK_Union from CXXClassRecord::classof, use std::string for AccessControl, style and remove unused imports
Add C++ class tests, fix exception spec fragments
- Add tests for single and multiple inheritance, class methods, and functions with noexcept.
- Exception spec fragments had a space included in it, moved it to separate fragment.
There aren't any variables or fields in the tests currently, waiting on D154038 because of no semicolons in their fragments.
clang/lib/ExtractAPI/API.cpp | ||
---|---|---|
19 | This include doesn't need to be here, clangd auto-included it. Using FunctionSignature would've explicitly included it in the first place. DeclarationFragments.h is already included in API.h. I was following FunctionSignature as far as where to declare and define. Moving AccessControl would mean including API.h in DeclarationFragments.h to allow DeclarationFragmentsBuilder::getAccessControl. | |
clang/lib/ExtractAPI/Serialization/SymbolGraphSerializer.cpp | ||
607 | This is done on line 534. "public" is serialized if the returned optional is empty. |
Looking pretty good, if you can address the last few bits of feedback I am happy to merge this.
clang/include/clang/ExtractAPI/API.h | ||
---|---|---|
770 | Does CXXInstanceMethodRecord need one of these as well? | |
clang/include/clang/ExtractAPI/DeclarationFragments.h | ||
25 | Are these necessary? | |
187 | For ergonomics would it make more sense to have a constructor that takes the string in directly and moves it to Access? I don't think there are any situation where you want to change the access string after the fact? | |
246 | I assume the implementation of this moved because it was needed for C++ methods? Can you update the comment and maybe move the implementation to the bottom of the .h file? | |
clang/lib/ExtractAPI/DeclarationFragments.cpp | ||
617 | Please address this, it's pretty important imo. | |
clang/test/ExtractAPI/methods.cpp | ||
14 | I think these are redundant since you have // expected-no-diagnostics below and pass `--verify` to cc1. |
Address review feedback, add tests, and small refactors.
- Add tests for destructors, constructors, and conversion methods.
- Remove redundant check-nots in existing tests.
- Properly record destructor names.
- Add explicit qualifier to conversions.
- Remove unused code from getFragmentsForSpecialCXXMethod.
- Use appendSpace isntead of literals in keyword fragments.
- Initialize AccessControl with access string instead of using setter.
clang/include/clang/ExtractAPI/API.h | ||
---|---|---|
770 | CXXInstanceMethodRecord inherits from CXXMethodRecord which holds the signature for all methods. |
Hi, we are seeing a test error on Clang :: ExtractAPI/constructor_destructor.cpp after this patch was landed. Error message:
Script: -- : 'RUN: at line 1'; rm -rf /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp : 'RUN: at line 2'; split-file /b/s/w/ir/x/w/llvm-llvm-project/clang/test/ExtractAPI/constructor_destructor.cpp /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp : 'RUN: at line 3'; sed -e "s@INPUT_DIR@/b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp@g" /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/reference.output.json.in >> /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/reference.output.json : 'RUN: at line 5'; /b/s/w/ir/x/w/llvm_build/bin/clang++ -extract-api -target arm64-apple-macosx -x c++-header /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/input.h -o /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/output.json -Xclang -verify : 'RUN: at line 9'; sed -e "s@\"generator\": \".*\"@\"generator\": \"?\"@g" /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/output.json >> /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/output-normalized.json : 'RUN: at line 11'; diff /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/reference.output.json /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/output-normalized.json -- Exit Code: 1 Command Output (stderr): -- clang++: llvm/include/llvm/Support/JSON.h:317: llvm::json::Value::Value(std::string): Assertion `false && "Invalid UTF-8 in value used as JSON"' failed. PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace, preprocessed source, and associated run script. Stack dump: 0. Program arguments: /b/s/w/ir/x/w/llvm_build/bin/clang++ -extract-api -target arm64-apple-macosx -x c++-header /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/input.h -o /b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/constructor_destructor.cpp.tmp/output.json -Xclang -verify #0 0x000055a2ec85c458 llvm::sys::PrintStackTrace(llvm::raw_ostream&, int) (/b/s/w/ir/x/w/llvm_build/bin/clang+++0x83e5458) clang++: error: clang frontend command failed with exit code 134 (use -v to see invocation) Fuchsia clang version 18.0.0 (https://llvm.googlesource.com/llvm-project dad9de0ae5360b18c890985d212bec266bf8c122) Target: arm64-apple-macosx Thread model: posix InstalledDir: /b/s/w/ir/x/w/llvm_build/bin clang++: note: diagnostic msg: ******************** PLEASE ATTACH THE FOLLOWING FILES TO THE BUG REPORT: Preprocessed source(s) and associated run script(s) are located at: clang++: note: diagnostic msg: /b/s/w/ir/x/t/lit-tmp-0iloyqe3/input-5086dc.hh clang++: note: diagnostic msg: /b/s/w/ir/x/t/lit-tmp-0iloyqe3/input-5086dc.sh clang++: note: diagnostic msg: ******************** --
Link to the builder: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-linux-x64/b8773854265208228753/overview
It looks like it triggers an assertion error in llvm's JSON library. Could you take a look please? If it takes some time to fix, could you revert your change please? Thanks.
Thanks for reverting. I wasn't doing any changes to this when you were asking though, but looking at build log, it looks like the assertion error disappeared after f4de606ef271fe362f03d30c53a850f9877ec238 was landed.
@haowei Thank you for letting me know! That patch does seem to fix it, unfortunately it didn't fail on my end. Thanks again.
FYI, this patch fails the Windows bots in a different way. (you can see the error triggered in the pre-merge build in https://buildkite.com/llvm-project/premerge-checks/builds/161603#018909c4-fe90-4de6-91a3-d251dc5ada8f)
The error message from our builders are:
Script: -- : 'RUN: at line 1'; rm -rf C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp : 'RUN: at line 2'; split-file C:\b\s\w\ir\x\w\llvm-llvm-project\clang\test\ExtractAPI\class.cpp C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp : 'RUN: at line 3'; sed -e "s@INPUT_DIR@C:/b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/class.cpp.tmp@g" C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/reference.output.json.in >> C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/reference.output.json : 'RUN: at line 5'; c:\b\s\w\ir\x\w\llvm_build\bin\clang.exe++ -extract-api -target arm64-apple-macosx -x c++-header C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/input.h -o C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/output.json -Xclang -verify : 'RUN: at line 9'; sed -e "s@\"generator\": \".*\"@\"generator\": \"?\"@g" C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/output.json >> C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/output-normalized.json : 'RUN: at line 11'; diff C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/reference.output.json C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/output-normalized.json -- Exit Code: 127 Command Output (stdout): -- $ ":" "RUN: at line 1" $ "rm" "-rf" "C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp" $ ":" "RUN: at line 2" $ "split-file" "C:\b\s\w\ir\x\w\llvm-llvm-project\clang\test\ExtractAPI\class.cpp" "C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp" $ ":" "RUN: at line 3" $ "sed" "-e" "s@INPUT_DIR@C:/b/s/w/ir/x/w/llvm_build/tools/clang/test/ExtractAPI/Output/class.cpp.tmp@g" "C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/reference.output.json.in" $ ":" "RUN: at line 5" $ "c:\b\s\w\ir\x\w\llvm_build\bin\clang.exe++" "-extract-api" "-target" "arm64-apple-macosx" "-x" "c++-header" "C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/input.h" "-o" "C:\b\s\w\ir\x\w\llvm_build\tools\clang\test\ExtractAPI\Output\class.cpp.tmp/output.json" "-Xclang" "-verify" # command stderr: 'c:\\b\\s\\w\\ir\\x\\w\\llvm_build\\bin\\clang.exe++': command not found error: command failed with exit status: 127 --
Link to the failed task: https://ci.chromium.org/ui/p/fuchsia/builders/toolchain.ci/clang-windows-x64/b8773853314698659521/overview
%clang++ is probably not the correct way to invoke clang++ as it was replaced as clang.exe++ on Windows
Inste