[PATCH] D128440: [WebAssembly] Initial support for reference type funcref in clang

[Paulo Matos via Phabricator via cfe-commits]

pmatos marked 6 inline comments as done.
pmatos added inline comments.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6552-6555
+  // The call we get looks like
+  // CallExpr
+  // `- ImplicitCastExpr
+  //   `- DeclRefExpr
----------------
tlively wrote:
> How do these parts correspond to the original source code?
If I understand your question correctly, this is just the semantic checks for each of the builtins.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6564
+  // Prepare FDecl type
+  QualType Pointee = Context.getFunctionType(Context.VoidTy, {}, {});
+  QualType Type = Context.getPointerType(Pointee);
----------------
aaron.ballman wrote:
> Why are you guaranteed this function returns void?
I think this question is not relevant anymore given the current code, right? The wasm ref null builtin will always return the null funcref.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6704-6710
+  // Therefore we need to change the types of the DeclRefExpr (stored in FDecl)
+  // and regenerate a straight up CallExpr on the modified FDecl.
+  // returning
+  // CallExpr
+  // `- FunctionDecl
+
+  // Prepare FDecl type
----------------
aaron.ballman wrote:
> These comments no longer seem to match the code -- nothing creates a `FunctionDecl` here.
Right - fixed.


================
Comment at: clang/lib/Sema/SemaChecking.cpp:6708
+  // CallExpr
+  // `- FunctionDecl
+
----------------
erichkeane wrote:
> All those comments don't really seem relevant here, and are not really correct.  The only thing SemaChecking should do is either modify its arguments (though you don't have any), or set its return type.  
Right, these comments can be removed. I fixed these.


================
Comment at: clang/lib/Sema/SemaType.cpp:7340
+  attr::Kind NewAttrKind = A->getKind();
+  QualType Desugared = Type;
+  const auto *AT = dyn_cast<AttributedType>(Type);
----------------
erichkeane wrote:
> This name seems misleading... should you be desugaring 'Type' to store it in 'Desugared'?
> 
> Also, I see that this value is never used, so why do the copy init?  ALSO, why is it called Desugared, when it never seems to get a Desugared type?  
Bad naming due to earlier code. I have simplified this code now.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D128440