[PATCH] D112579: Allow non-variadic functions to be attributed with `__attribute__((format))`

[Aaron Ballman via Phabricator via cfe-commits]

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