[PATCH] D27054: Introducing clang::tooling::EditList for refactoring tools.

[Daniel Jasper via Phabricator via cfe-commits]

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