[PATCH] D72448: [clang-tidy] readability-redundant-string-init now flags redundant initialisation in Field Decls and Constructor Initialisers

[Aaron Ballman via Phabricator via cfe-commits]

aaron.ballman added a comment.

In D72448#1844320 <https://reviews.llvm.org/D72448#1844320>, @njames93 wrote:

> In D72448#1844309 <https://reviews.llvm.org/D72448#1844309>, @aaron.ballman wrote:
>
> > In D72448#1844032 <https://reviews.llvm.org/D72448#1844032>, @njames93 wrote:
> >
> > > Hmm a lot of the code in the redundant-string-init check is designed to be macro unsafe. Not sure the best way to follow up, discard the old macro behaviour or keep it
> >
> >
> > Designed to be macro unsafe, or just didn't consider macros in the first place? I'm not seeing anything that makes me think macros were taken into account, but maybe you're looking at something different from me. Do you have an example of where you think something was designed to be macro unsafe?
>
>
> This is one of the test cases
>
>   #define M(x) x
>   #define N { std::string s = ""; }
>   // CHECK-FIXES: #define N { std::string s = ""; }
>  
>   void h() {
>     templ<int>();
>     templ<double>();
>  
>     M({ std::string s = ""; })
>     // CHECK-MESSAGES: [[@LINE-1]]:19: warning: redundant string initialization
>     // CHECK-FIXES: M({ std::string s; })
>  
>     N
>     // CHECK-MESSAGES: [[@LINE-1]]:3: warning: redundant string initialization
>     // CHECK-FIXES: N
>     N
>     // CHECK-MESSAGES: [[@LINE-1]]:3: warning: redundant string initialization
>     // CHECK-FIXES: N
>   }
>   // further down
>   #define EMPTY_STR ""
>   void j() {
>     std::string a(EMPTY_STR);
>     // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
>     // CHECK-FIXES: std::string a;
>     std::string b = (EMPTY_STR);
>     // CHECK-MESSAGES: [[@LINE-1]]:15: warning: redundant string initialization
>     // CHECK-FIXES: std::string b;
>
>
> Looks like they knew the behaviour they wanted back then


Looking at the source code for the check, this looks more like they were documenting the existing behavior rather than making a design choice. I definitely disagree with some of those fixits. It's your call whether you want to do work in this area or not, but I'm not convinced the original design is what we want today (but finding out more from the code reviews that added the test cases may bring up other information we're not considering right now).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D72448