[libcxx-commits] [PATCH] D144994: [Draft][libc++][modules] Adds std module.
Mark de Wever via Phabricator via libcxx-commits
libcxx-commits at lists.llvm.org
Fri Apr 14 10:32:42 PDT 2023
Mordante marked 5 inline comments as done.
Mordante added a comment.
Thanks for the review!
================
Comment at: libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp:92
+
+static bool is_vaiable_declaration(const clang::NamedDecl* decl) {
+ // Declarations nested in records are automatically exported with the record itself.
----------------
philnik wrote:
>
Thanks I already spotted this locally.
================
Comment at: libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp:102-106
+ if (clang::CXXMethodDecl::classof(decl))
+ return false;
+
+ if (clang::CXXDeductionGuideDecl::classof(decl))
+ return false;
----------------
philnik wrote:
> I think that's the idiomatic way to do this.
Interesting I wasn't aware `llvm::isa` multiple arguments. For now I keep it as is, but leave this comment open,
Once the code is working as intended I will see whether this change improves the code.
================
Comment at: libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp:141-156
+
+/// Returns the name is a reserved name.
+///
+/// This test misses 2 candidates which are not used in libc++
+/// * any identifier with two underscores not at the start
+/// * a name with a leading underscore in the global namespace
+bool is_reserved_name(const std::string& name) {
----------------
philnik wrote:
> We have something very similar in `ugilfy_attributes.cpp`. Maybe we should add a header for a few utilities?
That might be useful. BTW `if (str[0] == '_' && str[1] >= 'A' && str[1] <= 'Z'` does not work properly in EBCDIC.
================
Comment at: libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp:165
+
+ std::string name = get_qualified_name(*decl);
+ if (is_reserved_name(name))
----------------
philnik wrote:
> Is this equivalent to `decl->getName()`?
The documentation in not fully clear, but since `getName()` returns a `StringRef` I'm sure it's not the same.
`getName()` would return `fill` where `get_qualified_name()` returns `std::fill` or `std::ranges::fill`.
================
Comment at: libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.cpp:172
+
+ std::cout << "using " << std::string{name} << ";\n";
+ decls_.insert(name);
----------------
philnik wrote:
> What is this doing?
Dumping a line like `using std::vector`. This checker is not a real checker, but extracts information.
The output of a toplevel header is compared with the output of a module.
This is done in `libcxx/test/libcxx/module_std.sh.cpp` (which very much under review and I'm still looking at the failures.)
================
Comment at: libcxx/test/tools/clang_tidy_checks/header_exportable_declarations.hpp:21
+ const llvm::StringRef extra_header_;
+ std::set<std::string> decls_;
+};
----------------
philnik wrote:
> Would an `unordered_set` be better?
I haven't tested. Most headers don't have a lot of declarations. The module `type_traits` which is one of the larger modules has less than 300 entries. (In total there are less than 4000 entries in the Standard library.)
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D144994/new/
https://reviews.llvm.org/D144994
More information about the libcxx-commits
mailing list