[PATCH] D112374: [clang] Implement ElaboratedType sugaring for types written bare

[Richard Smith - zygoloid via Phabricator via cfe-commits]

rsmith added a comment.
Herald added a project: All.

I've not looked at the test changes in any detail; please let me know if there's anything in there that deserves special attention.



================
Comment at: clang-tools-extra/clang-change-namespace/ChangeNamespace.cpp:567-577
-    if (Loc.getTypeLocClass() == TypeLoc::Elaborated) {
-      NestedNameSpecifierLoc NestedNameSpecifier =
-          Loc.castAs<ElaboratedTypeLoc>().getQualifierLoc();
-      // This happens for friend declaration of a base class with injected class
-      // name.
-      if (!NestedNameSpecifier.getNestedNameSpecifier())
-        return;
----------------
Can we keep at least the second check here?


================
Comment at: clang-tools-extra/clang-tidy/readability/StaticAccessedThroughInstanceCheck.h:28
         NameSpecifierNestingThreshold(
-            Options.get("NameSpecifierNestingThreshold", 3U)) {}
+            Options.get("NameSpecifierNestingThreshold", 2U)) {}
 
----------------
If this is an externally-visible option that might be in someone's config file, can we keep it meaning the same thing? Eg, start with a `NestingLevel` of `1`, not `0`, so it counts the number of name components rather than the number of `::`s.


================
Comment at: clang/include/clang/AST/TypeLoc.h:2255-2257
+  unsigned getExtraLocalDataSize() const {
+    return !isEmpty() ? sizeof(ElaboratedLocInfo) : 0;
+  }
----------------
Instead of using a `LocalData` type of `void` and adding `llvm::SizeOf` etc, how about using a local data type of `ElaboratedLocInfo` and overriding `getLocalDataSize` to sometimes return 0 if you don't actually want to store local data? `AdjustedTypeLoc` does something similar already.


================
Comment at: clang/lib/AST/FormatString.cpp:979-983
+  for (const TypedefNameDecl *Typedef;; QT = Typedef->getUnderlyingType()) {
+    const auto *TT = QT->getAs<TypedefType>();
+    if (!TT)
+      return false;
+    Typedef = TT->getDecl();
----------------
Optional, but I think this is a bit clearer. (You'd need to re-add the `return false;` after the loop too.)


================
Comment at: clang/lib/CodeGen/CGCall.cpp:2790
           if (!AVAttr)
-            if (const auto *TOTy = dyn_cast<TypedefType>(OTy))
+            if (const auto *TOTy = OTy->getAs<TypedefType>())
               AVAttr = TOTy->getDecl()->getAttr<AlignValueAttr>();
----------------
Hm, this and the checks for this attribute in CGExprScalar.cpp aren't fully fixed: if we don't find the attribute on a typedef we should look inside that typedef to see if its target is another typedef. See `TypeHasMayAlias` for an example of code doing it correctly. Might make sense to land that as a separate change since it's a pre-existing bug?


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2210
     case Type::ExtInt:
       llvm_unreachable("type class is never variably-modified!");
 
----------------
Hm, this seems to already be reachable for an `ElaboratedType`, even without this change:

```
void f(int n) {
  using T = int[n];
  struct A {
    using U = T;
  };
  A::U au;
}
```

... but I think that's actually a bug. Bad things are going to happen if the members of `A` mention `U`.

Filed that as https://github.com/llvm/llvm-project/issues/55686.


================
Comment at: clang/lib/Sema/SemaTemplate.cpp:3982
+  // Create an elaborated-type-specifier containing the nested-name-specifier.
+  QualType ElTy = getElaboratedType(ETK_None, SS, SpecTy);
+  ElaboratedTypeLoc ElabTL = TLB.push<ElaboratedTypeLoc>(ElTy);
----------------
I think in the case we have a constructor or destructor name, we should pass in an empty scope specifier here, so that pretty-printing `Namespace::Class::Class() {}` produces just that instead of `Namespace::Class::Namespace::Class) {}`. (The first name specifier comes from printing the declaration's qualified name, and the second one would come from printing the constructor type.)


================
Comment at: llvm/include/llvm/Support/SizeOf.h:20-23
+/// A sizeof operator stand-in which supports `sizeof(void) == 0`.
+///
+template <class T> struct SizeOf { static constexpr size_t value = sizeof(T); };
+template <> struct SizeOf<void> { static constexpr size_t value = 0; };
----------------
I think it's too surprising to call this simply `SizeOf`, especially given that under https://www.open-std.org/jtc1/sc22/wg21/docs/papers/2016/p0146r1.html we might eventually see C++ defining `sizeof(void)` as some non-zero value.


================
Comment at: llvm/include/llvm/Support/SizeOf.h:25-32
+/// An alignof operator stand-in which supports `alignof(void) == 1`.
+///
+template <class T> struct AlignOf {
+  static constexpr size_t value = alignof(T);
+};
+template <> struct AlignOf<void> { static constexpr size_t value = 1; };
+
----------------
There's already a (different!) `llvm::AlignOf` in `llvm/Support/AlignOf.h`.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112374