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

[Conrad Poelman via Phabricator via cfe-commits]

poelmanc added a comment.

In D69238#1736365 <https://reviews.llvm.org/D69238#1736365>, @NoQ wrote:

> I suspect that it's not just the source range, but the whole AST for the initializer is different, due to C++17 mandatory copy elision in the equals-initializer syntax. Like, before C++17 it was a temporary constructor, a temporary materialization (ironic!), and a copy constructor, but in C++17 and after it's a single direct constructor which looks exactly like the old temporary constructor (except not being a `CXXTemporaryObjectExpr`). You're most likely talking about //different construct-expressions// in different language modes.
>
> That said, it should probably be possible to change the source range anyway somehow.


Thanks to all the encouragement here, I spent a few more hours stepping through code and have found a one-line change to clang\lib\Sema\SemaInit.cpp:8053 that fixes this bug! Change:

  SourceLocation Loc = CurInit.get()->getBeginLoc();

to

  SourceLocation Loc = Kind.getLocation();

For `SK_UserConversion`, `CurInit` is set at SemaInit.cpp:7899 to `Args[0]`, i.e. the first argument to the constructor, which is `""` in this case. By changing `Loc` to `Kind.getLocation()`, the `BuildCXXConstructExpr` at SemaInit.cpp:8064 gets created with a SourceRange spanning `a = ""`. With just that change, the SourceRange for an expression like `std::string a = ""` becomes consistent across C++11/14/17/2a and `readability-redundant-string-init` tests pass in all C++ modes (so we can throw away my 70 lines of manual parsing code.)

I have no experience with the clang parsing code so I don't know what other effects this change might have or who else we might want to check with before committing this. Should I change my "diff" here to just that?

> Also i don't see any tests for multiple declarations in a single `DeclStmt`, i.e.
> 
>   string A = "a", B = "";
> 
> 
> ... - which source range do you expect to have if you warn on `B`?

For that example I'd expect the warning source range to cover `B = ""`. `readability-redundant-string-init.cpp` does include tests almost like that: `std::string a = "", b = "", c = "";` warns on 3 separate source ranges, while `std::string d = "u", e = "u", f = "u";` doesn't warn at all. I'm happy to add a line that has some of each. I just added `std::string g = "u", h = "", i = "uuu", j = "", k;` and confirmed that it warns and fixes correctly (in C++11/14 mode it should pass already; in C++17/2a mode it works correctly with the current patch.)


Repository:
  rCTE Clang Tools Extra

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

https://reviews.llvm.org/D69238