[PATCH] D69238: Fix clang-tidy readability-redundant-string-init for c++17/c++2a

Tue Oct 29 14:14:02 PDT 2019

poelmanc marked 2 inline comments as done.
poelmanc added inline comments.

================
Comment at: clang-tools-extra/clang-tidy/readability/RedundantStringInitCheck.cpp:67-70
+// With -std c++14 or earlier (!LangOps.CPlusPlus17), it was sufficient to
+// return CtorExpr->getSourceRange(). However, starting with c++17, parsing
+// the expression 'std::string Name = ""' results in a CtorExpr whose
+// SourceRange includes just '""' rather than the previous 'Name = ""'.
----------------
aaron.ballman wrote:
> Are you sure that this behavioral difference isn't just a bug in Clang? I can't think of why the source range should differ based on language mode, so I wonder if the correct thing to do is fix how we calculate the source range in Clang?
Thanks and no, I'm not sure at all. At the end of my summary paragraph I wrote:

> Or, is it possible that this change in SourceRange is an unintentional difference in the parsing code? If so fixing that might make more sense.

Before starting this patch, I built a Debug build of LLVM and stepped through LLVM parsing code while running clang-tidy on the readability-redundant-string-init.cpp file. I set breakpoints and inspected variable values under both -std c++14 and -std c++17. The call stacks were clearly different. I could sometimes inspect character positions in the file and see where an expression's SourceLoc was set. However, a SourceLoc is a single character position; I couldn't figure out where an expression's SourceRange was even set. So I gave up and went down the current approach.

Should we ping the LLVM mailing list and see if this change rings a bell with anyone there?

Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D69238