djasper added inline comments.
================ Comment at: include/clang/Tooling/Refactoring/EditList.h:41 + /// \brief Creates an edit list for a key position. + EditList(const SourceManager &SM, SourceLocation KeyPosition); + ---------------- I wonder whether we should always use a SourceLocation as key or whether we want to leave this up to the users. E.g. we could make this take a string parameter and provide a string getKeyForLocation(const SourceManager &SM, SourceLocation KeyPosition); ================ Comment at: include/clang/Tooling/Refactoring/EditList.h:43 + + EditList(std::string Key, std::string FilePath, std::string Error, + std::vector<std::string> InsertedHeaders, ---------------- Does this need to be public? Specifically, couldn't the YamlTraits create a NormalizedEditList and then convertFromYAML can create the EditList object, which should have access to a private constructor? ================ Comment at: include/clang/Tooling/Refactoring/EditList.h:82-94 + /// \brief Adds a replacement that inserts \p Text at \p Loc. If this + /// insertion conflicts with an existing insertion (at the same position), + /// this will be inserted before the existing insertion. If the conflicting + /// replacement is not an insertion, an error is returned. + /// + /// \returns An llvm::Error carrying ReplacementError on error. + llvm::Error insertBefore(const SourceManager &SM, SourceLocation Loc, ---------------- Do we currently actually need these functions? They seem a bit dangerous. ================ Comment at: include/clang/Tooling/Refactoring/EditList.h:117 + + std::string Key; + std::string FilePath; ---------------- Maybe add a short description of what "Key" contains. The others are pretty intuitive. ================ Comment at: lib/Tooling/Refactoring/EditList.cpp:25 + /// access to its data members. + struct NormalizedEditList { + NormalizedEditList(const IO &) {} ---------------- I don't understand (without more comments) why this is normalized/denormalized. ================ Comment at: unittests/Tooling/RefactoringTest.cpp:1113 + llvm::Error Err = Edit.insertBefore(Context.Sources, DefaultLoc, "aa"); + EXPECT_TRUE(!Err); + Err = ---------------- I think these need to be ASSERT_TRUE. https://reviews.llvm.org/D27054 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits