stmuench wrote:

> Release notes entry missing.
> 
> Now, to be honest I do not like idea behind those changes. Simply because if 
> diagnostic is raised for example for an surrogate type, there is no way to 
> apply fixes, and as this check is "modernize", main purpose of it is to apply 
> fixes.
> 
> Personally I run into situation in my code base where this check is too 
> aggressive and should be relaxed.
> 
> Example:
> 
> ```
> #include <type_traits>
> 
> template <typename T, bool = std::is_same_v<int, T>> void f(T &&value) {}
> 
> void test() {
>   int t[10];
>   f(t);
> }
> ```
> 
> ```
> /root/1.cpp:3:50: warning: do not declare C-style arrays, use 'std::array' 
> instead 
> [cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays]
>     3 | template <typename T, bool = std::is_same_v<int, T>> void f(T 
> &&value) {}
>       |                                                  ^
> /root/1.cpp:6:3: warning: do not declare C-style arrays, use 'std::array' 
> instead 
> [cppcoreguidelines-avoid-c-arrays,hicpp-avoid-c-arrays,modernize-avoid-c-arrays]
>     6 |   int t[10];
>       |   ^
> ```
> 
> In above code check is executed in template instance even that 
> TK_IgnoreUnlessSpelledInSource is set. With your change it will be even worst.
> 
> Thing is that if check would properly handled implicit code then "template 
> parameters" would never be catch. At the end check should point places in 
> code where actually arrays are defined, otherwise if you would put array into 
> type alias, then you would get warning in every place that type alias is 
> used, and that's stupid as there is only one place where such array can be 
> fixed, and that is a definition of that type alias.
> 
> Due to above I do not like this change.

Many thanks for your prompt review and pointing out the flaws in my 
implementation. I fully agree that solely for modernization, this new setting 
in conjunction with the increased amount of diagnostics would not make sense. 
And that's why it's disabled by default and people would have to opt-in 
explicitly.
The main intention of this new mode was to help people recognize usages of 
C-style arrays in their code even where it's not clear immediately, e.g. when 
using types from externel headers. This would be very helpful when you are 
working in a project where the use of C-style arrays is discouraged or not even 
allowed.
Don't you think that this would be a helpful enhancement then?

https://github.com/llvm/llvm-project/pull/131468
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

Reply via email to