[PATCH] D158006: [Clang][WIP]Experimental implementation of data member packs in dependent context.

[David Blaikie via Phabricator via cfe-commits]

dblaikie added inline comments.


================
Comment at: clang/lib/Sema/SemaExprMember.cpp:519-521
+  if (const ElaboratedType *ET = dyn_cast<const ElaboratedType>(BaseType)) {
+    if (const TemplateSpecializationType *TST =
+            dyn_cast<TemplateSpecializationType>(ET->getNamedType())) {
----------------
I think you can skip the `const` in the first `dyn_cast` (think it works implicitly?) & could use `const auto *` on the LHS of both these `if`s, since the RHS makes it clear what the type is


================
Comment at: clang/lib/Sema/SemaExprMember.cpp:523-524
+      auto *TD = TST->getTemplateName().getAsTemplateDecl();
+      assert(isa<ClassTemplateDecl>(TD) &&
+             "template decl in member access is not ClassTemplateDecl");
+      for (FieldDecl *Field :
----------------
No need for the assert if you're immediately going to `cast` anyway, it'll assert. Though perhaps the custom assert here gives you a chance to make it more explicit that this is intentional.


================
Comment at: clang/lib/Sema/SemaExprMember.cpp:525-527
+      for (FieldDecl *Field :
+           cast<ClassTemplateDecl>(TD)->getTemplatedDecl()->fields()) {
+        if (Field->getDeclName() == NameInfo.getName()) {
----------------
This could be `llvm::find_if`, maybe? Not sure if it'd be tidier, but maybe more legible


================
Comment at: clang/lib/Sema/SemaExprMember.cpp:1023-1025
+    if (MemberNameStr.find('@') != std::string::npos) {
+      return ExprEmpty();
+    }
----------------
Usually don't put `{}` on single-line scopes in the LLVM codebase.


================
Comment at: clang/lib/Sema/SemaTemplateInstantiate.cpp:3292
+          std::string NewFieldName =
+              PackedField->getName().str() + "@" + std::to_string(Arg);
+          PackedField->setDeclName(&Context.Idents.get(NewFieldName));
----------------
cjdb wrote:
> Does LLVM have some form of `absl::StrCat` that we can use instead of `operator+`?
there's at least `llvm::Twine` which can avoid manifesting the intermediate strings


================
Comment at: clang/lib/Sema/TreeTransform.h:4171
 
+      if (CXXDependentScopeMemberExpr *MemberExpr =
+              dyn_cast<CXXDependentScopeMemberExpr>(Pattern)) {
----------------
Might be worth pulling out this new code as a separate function - the `continue` at the end is many lines away from the start or end of the loop, making control flow a bit hard to follow (probably the existing code could do with some of this massaging even before/regardless of the new code you're adding here)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D158006