[PATCH] D33735: [DebugInfo] Add ThisOrSelf attribute for emission of FlagObjectPointer.

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: include/clang/AST/Decl.h:901
+    /// member functions.
+    unsigned ImplicitParamKind : 3;
   };
----------------
It's a bit strange to me that the non-parameter declaration bits now have a field for implicit parameter information. Why here instead of `ParmVarDeclBits`?


================
Comment at: include/clang/AST/Decl.h:1383
 class ImplicitParamDecl : public VarDecl {
+public:
+  /// Defines the kind of the implicit parameter: is this an implicit parameter
----------------
Rather than use three access specifiers, can you reorder this?
```
class ... {
  void anchor() override;

public:
  ...
};
```


================
Comment at: lib/CodeGen/CGDebugInfo.cpp:3471
+  // then give it an object pointer flag.
+  if (auto *IPD = dyn_cast<ImplicitParamDecl>(VD)) {
+    if (IPD->getParameterKind() == ImplicitParamDecl::CXXThis ||
----------------
`const auto *` please.


================
Comment at: lib/CodeGen/CGDebugInfo.cpp:3586
   // block. Mark it as the object pointer.
-  if (isa<ImplicitParamDecl>(VD) && VD->getName() == "self")
-    Ty = CreateSelfType(VD->getType(), Ty);
+  if (auto *IPD = dyn_cast<ImplicitParamDecl>(VD))
+    if (IPD->getParameterKind() == ImplicitParamDecl::ObjCSelf)
----------------
`const auto *`


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:3467
   ImplicitParamDecl TaskPrivatesArg(
-      C, /*DC=*/nullptr, Loc, /*Id=*/nullptr,
-      C.getPointerType(PrivatesQTy).withConst().withRestrict());
+      C, C.getPointerType(PrivatesQTy).withConst().withRestrict(),
+      ImplicitParamDecl::Other);
----------------
This no longer sets the SourceLocation -- is that intended?


================
Comment at: lib/CodeGen/CGOpenMPRuntime.cpp:3479-3480
+        ImplicitParamDecl::Other);
+    IPD->setLocation(Loc);
+    Args.push_back(IPD);
     auto *VD = cast<VarDecl>(cast<DeclRefExpr>(E)->getDecl());
----------------
This code would be cleaner if ImplicitParamDecl::Create() took a SourceLocation as it originally did. Perhaps the last param could be a default argument that defaults to `SourceLocation{}`?


================
Comment at: lib/CodeGen/CGStmtOpenMP.cpp:284
     }
-    Args.push_back(ImplicitParamDecl::Create(getContext(), nullptr,
-                                             FD->getLocation(), II, ArgType));
+    auto *IPD = ImplicitParamDecl::Create(getContext(), /*DC=*/nullptr,
+                                          FD->getLocation(), II, ArgType,
----------------
Why use a local variable here?


================
Comment at: lib/CodeGen/ItaniumCXXABI.cpp:1411
     QualType T = Context.getPointerType(Context.VoidPtrTy);
-    ImplicitParamDecl *VTTDecl
-      = ImplicitParamDecl::Create(Context, nullptr, MD->getLocation(),
-                                  &Context.Idents.get("vtt"), T);
+    ImplicitParamDecl *VTTDecl = ImplicitParamDecl::Create(
+        Context, nullptr, MD->getLocation(), &Context.Idents.get("vtt"), T,
----------------
Can use `auto *` here.


================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1416
   if (isa<CXXConstructorDecl>(MD) && MD->getParent()->getNumVBases()) {
-    ImplicitParamDecl *IsMostDerived
-      = ImplicitParamDecl::Create(Context, nullptr,
-                                  CGF.CurGD.getDecl()->getLocation(),
-                                  &Context.Idents.get("is_most_derived"),
-                                  Context.IntTy);
+    ImplicitParamDecl *IsMostDerived = ImplicitParamDecl::Create(
+        Context, /*DC=*/nullptr, CGF.CurGD.getDecl()->getLocation(),
----------------
`auto *`


================
Comment at: lib/CodeGen/MicrosoftCXXABI.cpp:1429
   } else if (isDeletingDtor(CGF.CurGD)) {
-    ImplicitParamDecl *ShouldDelete
-      = ImplicitParamDecl::Create(Context, nullptr,
-                                  CGF.CurGD.getDecl()->getLocation(),
-                                  &Context.Idents.get("should_call_delete"),
-                                  Context.IntTy);
+    ImplicitParamDecl *ShouldDelete = ImplicitParamDecl::Create(
+        Context, /*DC=*/nullptr, CGF.CurGD.getDecl()->getLocation(),
----------------
`auto *`


================
Comment at: lib/Sema/SemaStmt.cpp:3959
   QualType ParamType = Context.getPointerType(Context.getTagDeclType(RD));
-  ImplicitParamDecl *Param
-    = ImplicitParamDecl::Create(Context, DC, Loc, ParamName, ParamType);
+  ImplicitParamDecl *Param =
+      ImplicitParamDecl::Create(Context, DC, Loc, ParamName, ParamType,
----------------
`auto *` (and elsewhere, I'll stop posting about them.)


================
Comment at: lib/Serialization/ASTWriterDecl.cpp:918
     Record.push_back(D->isPreviousDeclInSameBlockScope());
+    if (auto *IPD = dyn_cast<ImplicitParamDecl>(D))
+      Record.push_back(static_cast<unsigned>(IPD->getParameterKind()));
----------------
`const auto *`


https://reviews.llvm.org/D33735