[PATCH] D13549: Added new options to ClangFormat VSIX package.

[Zachary Turner via cfe-commits]

zturner added a comment.

No obvious problems, mostly just style issues.  Feel free to consider them optional.


================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:38
@@ +37,3 @@
+        private bool sortIncludes = false;
+        private string style = "file";
+
----------------
How about an enum instead?  

    private enum Style {
        File,
        Chromium,
        Google,
        LLVM,
        Mozilla,
        Webkit
    }
    private Style style;


Then change the `StyleConverter` to just convert the enum value to its corresponding string representation?

================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:42-47
@@ +41,8 @@
+        {
+            protected ArrayList values;
+            public StyleConverter()
+            {
+                // Initializes the standard values list with defaults.
+                values = new ArrayList(new string[] { "file", "Chromium", "Google", "LLVM", "Mozilla", "WebKit" });
+            }
+
----------------
This would all go away

================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:56
@@ +55,3 @@
+            {
+                return new StandardValuesCollection(values);
+            }
----------------
This becomes `return new StandardValuesCollection(Enum.GetValues(typeof(Style)))`

================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:61-64
@@ +60,6 @@
+            {
+                if (sourceType == typeof(string))
+                    return true;
+
+                return base.CanConvertFrom(context, sourceType);
+            }
----------------
This becomes `sourceType == typeof(Style) || base.CanConvertFrom(context, sourceType)`

================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:69-73
@@ +68,7 @@
+            {
+                string s = value as string;
+                if (s == null)
+                    return base.ConvertFrom(context, culture, value);
+
+                return value;
+            }
----------------
This becomes

    Style s = value as Style;
    if (value == null)
        return base.ConvertFrom(context, culture, value);
    return value.ToString();

(I think, I can't tell what the source and destination type are supposed to be here)

================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:92
@@ -51,2 +91,3 @@
+        [TypeConverter(typeof(StyleConverter))]
         public string Style
         {
----------------
Now you can return an enum here instead of a string

================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:115
@@ +114,3 @@
+                // Check if string contains quotes. On Windows, file names cannot contain quotes.
+                // We do not accept them however to avoid hard-to-debug problems.
+                // A quote in user input would end the parameter quote and so break the command invocation.
----------------
Wouldn't it be better to just return `false` from `CanConvertFrom` if the filename contains quotes?  Then you can assert in this function that it does not contain quotes?

================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:136-143
@@ +135,10 @@
+
+        public sealed class FallbackStyleConverter : StyleConverter
+        {
+            public FallbackStyleConverter()
+            {
+                // Add "none" to the list of styles.
+                values.Insert(0, "none");
+            }
+        }
+
----------------
Same comment as before, but use the enum.s  I guess you would need to make `None` one of the actual style enum values, but remove it before returning the list in the other StyleConverter.  

================
Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:331-347
@@ -213,1 +330,19 @@
 
+        private string GetAssumeFilename()
+        {
+            var page = (OptionPageGrid)GetDialogPage(typeof(OptionPageGrid));
+            return page.AssumeFilename;
+        }
+
+        private string GetFallbackStyle()
+        {
+            var page = (OptionPageGrid)GetDialogPage(typeof(OptionPageGrid));
+            return page.FallbackStyle;
+        }
+
+        private bool GetSortIncludes()
+        {
+            var page = (OptionPageGrid)GetDialogPage(typeof(OptionPageGrid));
+            return page.SortIncludes;
+        }
+
----------------
Maybe you could use properties instead of functions.


http://reviews.llvm.org/D13549