[libcxx-commits] [PATCH] D144994: [Draft][libc++][modules] Adds std module.

[Mark de Wever via Phabricator via libcxx-commits]

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