ilya-biryukov added a comment. Only have a few NITs, will dig deeper into the change tomorrow. Added @arphaman too as an owner of the index library. Alex, feel free to reassign if you're the wrong person to take a look at this
================ Comment at: unittests/Index/IndexTests.cpp:31 +struct Position { + size_t Line; + size_t Column; ---------------- NIT: initialize with 0 to avoid UB. ================ Comment at: unittests/Index/IndexTests.cpp:40 + Position P; + P.Line = static_cast<int>(SM.getLineNumber(FID, Offset)) - 1; + P.Column = SM.getColumnNumber(FID, Offset) - 1; ---------------- Why do we need to `static_cast` to int? Can we leave out the cast? ================ Comment at: unittests/Index/IndexTests.cpp:45 + + bool operator==(const Position &Other) const { + return std::tie(Line, Column) == std::tie(Other.Line, Other.Column); ---------------- NIT: maybe make it a free-standing function, accepting two parameters: ``` struct Position { friend bool operator==(const Pos& L, const Pos& R) { // ... } }; ``` Doesn't really matter much here, though, just a general best practice. ================ Comment at: unittests/Index/IndexTests.cpp:97 std::vector<TestSymbol> Symbols; + const ASTContext *AST; }; ---------------- NIT: initialize with null to make UB less likely Repository: rC Clang CHANGES SINCE LAST ACTION https://reviews.llvm.org/D58189/new/ https://reviews.llvm.org/D58189 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits