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

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added inline comments.


================
Comment at: clang/lib/Sema/SemaDecl.cpp:3220
+
+  // `type[]` is equivalent to `type *` and `type[*]`
+  if (NoSizeInfo(Old) && NoSizeInfo(New))
----------------



================
Comment at: clang/lib/Sema/SemaDecl.cpp:3224
+
+  // Don't try to compare VLA sizes, unless one of them has the star modifier
+  if (Old->isVariableArrayType() && New->isVariableArrayType()) {
----------------



================
Comment at: clang/lib/Sema/SemaDecl.cpp:3234
+
+  // Only compare size, ignore Size modifiers and CVR
+  if (Old->isConstantArrayType() && New->isConstantArrayType())
----------------



================
Comment at: clang/lib/Sema/SemaDecl.cpp:3274-3278
+      S.Diag(NewParam->getLocation(), diag::warn_inconsistent_array_form)
+          << NewParam->getName()
+          << NewParamOT->getCanonicalTypeInternal().getAsString();
+      S.Diag(OldParam->getLocation(), diag::note_previous_declaration_as)
+          << OldParamOT->getCanonicalTypeInternal().getAsString();
----------------
The diagnostics engine knows how to format `NamedDecl` objects as well as types. Can you do this instead and still get reasonable diagnostics?


================
Comment at: clang/test/Sema/array-parameter.c:2
+// RUN: %clang_cc1 -triple x86_64-pc-linux-gnu -fsyntax-only -Warray-parameter -verify %s
+
+void f0(int a[]);
----------------
aaron.ballman wrote:
> I'd like to see some additional test cases:
> ```
> void func(int *p);
> void func(int a[2]); // Diagnose this one, I presume?
> void func(int a[]); // But also diagnose this one as well, yes?
> void func(int a[2]) {} // Do we then diagnose this as well, or is this matching a previous declaration and thus fine?
> 
> void other(int n, int m, int a[n]);
> void other(int n, int m, int a[m]); // Hopefully we diagnose this set!
> 
> void another(int n, int array[n]);
> void another(int n, int array[*]); // I *think* this should be warned, but I'm still a bit on the fence about it
> ```
> 
> Also, if this is expected to work in C++ as well, we should probably have cases like:
> ```
> template <int N>
> void func(int i[10]);
> 
> template <int N>
> void func(int i[N]); // Should we diagnose this before instantiation or wait until we see the instantiation and only diagnose if differs?
> 
> static constexpr int Extent = 10;
> void other(int i[10]);
> void other(int i[Extent]); // Should not be diagnosed
> ```
Do you plan to add the C++ tests?


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

https://reviews.llvm.org/D128449