[PATCH] D46180: [clang-format] Refactor #include insertion/deletion functionality into a class.

[Ilya Biryukov via Phabricator via cfe-commits]

ilya-biryukov added a comment.

I somehow missed the review email, sorry for the delayed response.

Just one nit and one question from me on changed behavior in the tests (quoted vs angled #include).
Otherwise LG, just wanted to make sure the change in behavior is intentional.



================
Comment at: lib/Format/Format.cpp:2131
+inline StringRef trimInclude(StringRef IncludeName) {
+  return IncludeName.trim("\"<>");
+}
----------------
NIT: Maybe we could add some asserts to this function that the passed include name is actually properly quoted.
E.g. starts with '<' or '"', ends with the corresponding char, etc.

So long as this is part of `Format.cpp`, it's not terribly important, since it's only called on paths that come from regex matches.
If we make it a public later, having asserts would be really useful.
That said, we might address it later when/if we actually make this function public.


================
Comment at: unittests/Format/CleanupTest.cpp:862
   std::string Expected = "#include \"a.h\"\n"
-                         "#include \"vector\"\n"
-                         "#include <vector>\n"
-                         "#include <a.h>\n";
+                         "#include <vector>\n";
   tooling::Replacements Replaces =
----------------
Are we sure we want this behavior change?
The API seems to allow us keeping the old one.

I know there's a FIXME there, but I just wanted to understand better what's the right thing to do here and why.


Repository:
  rC Clang

https://reviews.llvm.org/D46180