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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
