aaron.ballman added a comment. In D112579#3603629 <https://reviews.llvm.org/D112579#3603629>, @fcloutier wrote:
> Would it be better if I asked a colleague to finish the review? Typically, you should try to get a LG from the reviewers who have been active on the review in the past (assuming they're still active in the community now). So no -- It just takes a while because there's a lot of review work to be done and only so many hours in the day; sorry for the delays! I think there are some missing changes to AttrDocs.td for the new functionality, and this should have a release note as well. ================ Comment at: clang/include/clang/Basic/DiagnosticSemaKinds.td:4126 +def warn_gcc_requires_variadic_function : Warning< + "GCC requires functions with the %0 attribute to be variadic">, + InGroup<GccCompat>; ---------------- Slight tweaks: `GCC requires a function with the 'format' attribute to be variadic` ================ Comment at: clang/lib/AST/FormatString.cpp:327 + // format consumer. Apply decay before type comparison. + if (C.getAsArrayType(argTy) || argTy->isFunctionType()) + argTy = C.getDecayedType(argTy); ---------------- I think this should be: ``` if (argTy->canDecayToPointerType()) argTy = C.getDecayedType(argTy); ``` ================ Comment at: clang/lib/Sema/SemaChecking.cpp:5434-5440 + if (Format->getFirstArg() == 0) { + FSI->ArgPassingKind = FAPK_VAList; + } else if (IsVariadic) { + FSI->ArgPassingKind = FAPK_Variadic; + } else { + FSI->ArgPassingKind = FAPK_Fixed; + } ---------------- Elide braces here (coding style rule). ================ Comment at: clang/lib/Sema/SemaChecking.cpp:8622-8623 + // + if (const ParmVarDecl *PV = dyn_cast<ParmVarDecl>(VD)) { + if (const Decl *D = dyn_cast<Decl>(PV->getDeclContext())) { + for (const auto *PVFormat : D->specific_attrs<FormatAttr>()) { ---------------- Can use `const auto *` in these cases. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:8626 + bool IsCXXMember = false; + if (const CXXMethodDecl *MD = dyn_cast<CXXMethodDecl>(D)) + IsCXXMember = MD->isInstance(); ---------------- Same here. ================ Comment at: clang/lib/Sema/SemaChecking.cpp:8630-8631 + bool IsVariadic = false; + if (const FunctionType *FnTy = D->getFunctionType()) + IsVariadic = cast<FunctionProtoType>(FnTy)->isVariadic(); + else if (const auto *BD = dyn_cast<BlockDecl>(D)) ---------------- A better way to write this would be: ``` if (const auto *FnTy = D->getType()->getAs<FunctionProtoType>()) IsVariadic = FnTy->isVariadic(); ... ``` ================ Comment at: clang/lib/Sema/SemaChecking.cpp:10027 + // format consumer. Apply decay before type comparison. + if (S.Context.getAsArrayType(ExprTy) || ExprTy->isFunctionType()) + ExprTy = S.Context.getDecayedType(ExprTy); ---------------- Same suggestion here as above to use `canDecayToPointerType()` instead. ================ Comment at: clang/lib/Sema/SemaDeclAttr.cpp:3881-3885 if (isFunctionOrMethodVariadic(D)) { ++NumArgs; // +1 for ... } else { - S.Diag(D->getLocation(), diag::err_format_attribute_requires_variadic); - return; + S.Diag(D->getLocation(), diag::warn_gcc_requires_variadic_function) << AL; } ---------------- There's some braces you can elide here now. CHANGES SINCE LAST ACTION https://reviews.llvm.org/D112579/new/ https://reviews.llvm.org/D112579 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits