[PATCH] D128449: [clang] Introduce -Warray-parameter

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:324-326
+- Added the ``-Warray-parameter`` warning. It detects function redefinition,
+  where different definition involve argument type that decay to the same
+  pointer type from different array types.
----------------



================
Comment at: clang/lib/Sema/SemaDecl.cpp:3226-3227
+  if (Old->isVariableArrayType() && New->isVariableArrayType()) {
+    const auto *OldVAT = cast<ArrayType>(Old);
+    const auto *NewVAT = cast<ArrayType>(New);
+    if ((OldVAT->getSizeModifier() == ArrayType::ArraySizeModifier::Star) ^
----------------
Same here.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:3216
+           (Ty->isVariableArrayType() &&
+            cast<VariableArrayType>(Ty)->getSizeModifier() ==
+                ArrayType::ArraySizeModifier::Star);
----------------
aaron.ballman wrote:
> I forgot about this gotcha -- arrays are special in that you shouldn't be using `cast<>` and friends on them, you need to ask the `ASTContext` to go from the `QualType` to the correct array type. e.g., `ASTContext::getAsConstantArrayType()` and `ASTContext::getAsVariableArrayType()` -- I think this is the cause of the failed assertions we're seeing in precommit CI.
I'm still not comfortable with the way this is written -- please go through the `ASTContext` instead, with something like:
```
if (Ty->isIncompleteArrayType() || Ty->isPointerType())
  return true;

if (const auto *VAT = Ctx.getAsVariableArrayType(Ty))
  return VAT->getSizeModifier() == ArrayType::ArraySizeModifier::Star;

return false;
```



================
Comment at: clang/test/Sema/array-parameter.c:17
+
+void f5(int a[restrict 2]);
+void f5(int a[2]); // no warning
----------------
shafik wrote:
> Since we are covering `static`, `const` and `restict` we should also cover `volatile` for completeness. 
+1, might as well round out the set.


================
Comment at: clang/test/Sema/array-parameter.cpp:3-7
+template <int N>
+void func(int i[10]); // expected-note {{previously declared as 'int[10]' here}}
+
+template <int N>
+void func(int i[N]); // expected-warning {{argument 'i' of type 'int[N]' with mismatched bound}}
----------------
One more test, now that I'm thinking about explicit specializations:
```
template <int N>
void func(int (&Val)[N]);

template <>
void func<10>(int (&Val)[10]) {
}
```
I don't think this should get any diagnostics.


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

https://reviews.llvm.org/D128449