[PATCH] D29221: clang-format-vsix: "format on save" feature

[Hans Wennborg via Phabricator via cfe-commits]

hans added a comment.

I'm not really qualified to review the implementation details, but this seems good as far as I can tell. A few comments, mostly style related.



================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:39
         private string style = "file";
+        private bool formatOnSaveEnabled = false;
+        private string formatOnSaveFileExtensions =
----------------
Perhaps just `formatOnSave`, similar to `sortIncludes` above?


================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:46
+        {
+            // Use MemberwiseClone to copy value types
+            var clone = (OptionPageGrid)MemberwiseClone();
----------------
Ultra nit: end the sentence with a period.


================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:88
 
-        [Category("LLVM/Clang")]
+        [Category("Format Options")]
         [DisplayName("Style")]
----------------
What do you think about using "clang-format" for the category name?


================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:178
+
+        [Category("Format On Save")]
+        [DisplayName("Enable")]
----------------
Does this mean the "FormatOnSave" is not nested under the same category as the other clang-format options?


================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:276
+
+            if (Vsix.IsDocumentDirty(document))
+            {
----------------
Perhaps return early if `!Vsix.IsDocumentDirty(document)` instead?


https://reviews.llvm.org/D29221