[PATCH] D42520: Implement __attribute__((import_module("foo")))

[Aaron Ballman via Phabricator via llvm-commits]

aaron.ballman requested changes to this revision.
aaron.ballman added a reviewer: aaron.ballman.
aaron.ballman added a comment.

You're missing a lot of test cases for the semantic checks of the attribute in Clang. There should be a Sema test that ensures things like the attribute subject, its argument count and types, etc.



================
Comment at: tools/clang/include/clang/Basic/Attr.td:1367
+  let Args = [StringArgument<"ImportModuleName">];
+  let Documentation = [Undocumented];
+}
----------------
No new, undocumented attributes, please.


================
Comment at: tools/clang/lib/CodeGen/TargetInfo.cpp:738
+    TargetCodeGenInfo::setTargetAttributes(D, GV, CGM, IsForDefinition);
+    if (auto *FD = dyn_cast_or_null<FunctionDecl>(D)) {
+      if (auto *Attr = FD->getAttr<WebAssemblyImportModuleAttr>()) {
----------------
`const auto *` -- Also, can `D` really be null?


================
Comment at: tools/clang/lib/CodeGen/TargetInfo.cpp:739
+    if (auto *FD = dyn_cast_or_null<FunctionDecl>(D)) {
+      if (auto *Attr = FD->getAttr<WebAssemblyImportModuleAttr>()) {
+        llvm::Function *Fn = cast<llvm::Function>(GV);
----------------
`const auto *`


================
Comment at: tools/clang/lib/Sema/SemaDeclAttr.cpp:5437
+static void handleWebAssemblyImportModuleAttr(Sema &S, Decl *D, const AttributeList &Attr) {
+  if (!isFunctionOrMethod(D)) {
+    S.Diag(D->getLocation(), diag::warn_attribute_wrong_decl_type)
----------------
This should be specified by the `Subjects` list in Attr.td.


================
Comment at: tools/clang/lib/Sema/SemaDeclAttr.cpp:5445
+  if (FD->isThisDeclarationADefinition()) {
+    S.Diag(Attr.getLoc(), diag::err_alias_is_definition) << FD << 0;
+    return;
----------------
Is this function really an alias?


Repository:
  rL LLVM

https://reviews.llvm.org/D42520