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