Page MenuHomePhabricator

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

Authored by njames93 on Jan 9 2020, 6:24 AM.

Details

Summary

The original behaviour of this check only looked at VarDecls with strings that had an empty string initializer. This has been improved to check for FieldDecls with an in class initializer as well as constructor initializers.

Addresses clang-tidy "modernize-use-default-member-init"/"readability-redundant-string-init" and redundant initializer of std::string

Diff Detail

Event Timeline

njames93 created this revision.Jan 9 2020, 6:24 AM
njames93 marked an inline comment as done.Jan 9 2020, 6:26 AM
njames93 added inline comments.
clang-tools-extra/test/clang-tidy/checkers/readability-redundant-string-init.cpp
250

I'm aware the extra space between the Foo() and the braces is unsightly, but its something that clang-format would fix. Same goes for all other lines

njames93 edited the summary of this revision. (Show Details)Jan 9 2020, 6:29 AM
njames93 edited the summary of this revision. (Show Details)
Eugene.Zelenko added inline comments.
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
24

Please use static instead of anonymous namespaces. Same below.

26

Could it be const auto *?

149

Could it be const auto *?

161

Please don't use auto when type is not explicitly spelled in same statement or iterator.

njames93 updated this revision to Diff 237077.Jan 9 2020, 7:12 AM
njames93 marked 5 inline comments as done.

Address nits

clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
149

This shouldn't be auto anyway. Also the reason its `auto * and not const auto *` is because that's the qualified type getMember returns. But in this example as I'm not modifying I could add a const qualifier

It'll be reasonable to mention changes in Release Notes (in checks changes section, in alphabetical order).

njames93 updated this revision to Diff 237127.Jan 9 2020, 10:33 AM

updated release notes

njames93 updated this revision to Diff 237297.Jan 10 2020, 5:43 AM
njames93 edited the summary of this revision. (Show Details)

Checks for string initialisations which are spelt with braces

Ensures the check will detect and fix brace initializations
Example:

std::string a{""};
std::string b = {""};

CxxConstructor : A{""} {}

njames93 updated this revision to Diff 237363.Jan 10 2020, 9:45 AM

Tweaked a few documentation issues

whoops typo in arc, will fix, this is another patch

njames93 updated this revision to Diff 237368.Jan 10 2020, 9:50 AM

Undid my oopsie sending the wrong patch here

njames93 updated this revision to Diff 238764.Jan 17 2020, 6:39 AM
  • Updated to trunk, fix release notes

Unit tests: pass. 61955 tests passed, 0 failed and 783 were skipped.

clang-tidy: unknown.

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

njames93 updated this revision to Diff 239884.Jan 23 2020, 6:43 AM
  • Small refactor

Unit tests: pass. 62130 tests passed, 0 failed and 808 were skipped.

clang-tidy: fail. clang-tidy found 0 errors and 2 warnings. 0 of them are added as review comments below (why?).

clang-format: fail. Please format your changes with clang-format by running git-clang-format HEAD^ or applying this patch.

Build artifacts: diff.json, clang-tidy.txt, clang-format.patch, CMakeCache.txt, console-log.txt, test-results.xml

Pre-merge checks is in beta. Report issue. Please join beta or enable it for your project.

aaron.ballman added inline comments.Jan 25 2020, 11:29 AM
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
58

Should we be worried about macros here?

It looks a bit like we're ignoring macros entirely for this check, so maybe that can be done as a separate patch instead. The situation I am worried about is:

#if SOMETHING
#define INIT ""
#else
#define INIT "haha"
#endif

std::string S = INIT;
110

Should this also match on something like std::string foo{}; as a redundant init? Similar question for the other cases. (Could be done in a follow-up patch if desired.)

njames93 marked 2 inline comments as done.Sun, Jan 26, 1:36 PM
njames93 added inline comments.
clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
58

I feel that everything in this check needs to check macros, but that's probably a follow up patch

110

Probably should, however this is not what this patch is about

aaron.ballman accepted this revision.Mon, Jan 27, 1:49 PM
aaron.ballman marked 2 inline comments as done.

LGTM!

clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp
58

Okay, separate patch it is.

110

Okay, I'm fine with that as well.

This revision is now accepted and ready to land.Mon, Jan 27, 1:49 PM
This revision was automatically updated to reflect the committed changes.
njames93 marked 2 inline comments as done.Tue, Jan 28, 2:11 AM

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

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?

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

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).