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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
