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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits