Added new options to ClangFormat VSIX package.

[curdeius]

Summary

Added new options to ClangFormat VSIX package:

• fallback-style
• assume-filename
• sort-includes.

Changed version to 1.1 (otherwise one couldn't update).

Fixed clang-format escaping of XML reserved characters.

[djasper]

Please always add cfe-commits as "subscriber" so that the email also goes to the list.

[curdeius]

Escape XML-reserved characters.

[curdeius]

Fix description formatting.

[klimek]

lib/Driver/Tools.cpp

2356 ↗

(On Diff #37081)

Seems unrelated?

tools/clang-format/ClangFormat.cpp

229

For XML, we should only need "<" and "&". Everything else is just important in a context introduced by "<" or "&".
The problem with include-sorting is that we'll now need to fully support utf8 input (well, any encoding, to be precise).
The best way is probably to escape all code points that are outside the ascii range. The problem I see is that we have no idea what encoding the file is in, so I have no clue how we can really resolve this.

[curdeius]

lib/Driver/Tools.cpp

2356 ↗

(On Diff #37081)

Oups, I've made a mistake when updating the revision and pulled in the changes from master.
I'll correct this.

tools/clang-format/ClangFormat.cpp

229

Ok. I'll cut it down to the necessary minimum ('<', '&').
Anyway, I thing it's more urgent to fix the include sorting containing open brackets and later work on UTF-8 support.

[klimek]

tools/clang-format/ClangFormat.cpp

229

Agreed. Can you please add a FIXME though:
FIXME: When we sort includes, we need to make sure the stream is correct
utf-8.

[curdeius]

Escape only '<' and '&'.
Remove unrelated changes from the revision.

[curdeius]

Add FIXME comment.

[curdeius]

Applied suggestions from comments.

[klimek]

tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs

261

Don't we need " escaping for assumeFilename? (or do we consider that an error? in which case, would we want to make that an error?)

[curdeius]

tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs

261

Well, quotes (") are not allowed as a part of a file name on Windows.
What we could do is to strip surrounding quotes if the user added them in order not to add them twice.

[klimek]

tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs

261

Oh, I didn't know that.
My main concern is what happens when the user (accidentally) adds quotes (for example, unbalanced) and messes up the command line in a way that leads to a super hard to diagnose problem

[curdeius]

tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs

261

Hmm, you're right, we should do something with that.
Either:

• escape all quotes (but then the errors will be silent and user will not know that something bad happens)
• or to give an error message somehow, e.g. by using a custom TypeConverter for AssumeFilename option that will check for the existence of quotes.

[klimek]

tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs

261

I'd vote for the latter, if possible.

[curdeius]

Add option converters for filenames (prohibiting quotes) and styles (giving a list of predefined styles).

[curdeius]

Assume-Filename option will now give an error when there are quotes in the filename.

[klimek]

+aaron, as token Windows Wizard

+rnk, as I was told you might know people who are in a good position to review MS specific stuff (perhaps engineers from MS might be able to help here? :)

[aaron.ballman]

Generally looks good to me, with a few small nits.

tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs

56

Can we just return directly here instead of storing in a local?

81

Why is 'file' now lowercased?

115

I see now why this is being used, but it took a lot of context from review comments to understand. Can you please put in a more descriptive comment about why we're checking for quotes, despite them being invalid filename characters on Windows?

[curdeius]

tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs

56

Hmm, yes, we can.

81

I wanted to be consistent with clang-format --help. Besides, lowercase styles (file, none) are somewhat special and it's nice to have them stand out, IMO.

115

OK, will do.

[aaron.ballman]

tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs

81

That seems reasonable to me. Just making sure it wasn't a drive-by unintended change, or had some weird semantic meaning in MSVC.

[curdeius]

Applied Aaron's comments. Removed unused using.

[curdeius]

Applied comments and done some minor clean up.

[curdeius]

Ping?

[klimek]

We're waiting for Reid to find somebody who is good at reviewing this in detail.
Sorry it takes a while, so far we don't have enough trusted Windows experts in the community :(

[djasper]

While we are waiting, submitted the changes to ClangFormat.cpp in r250440. Thank you!

[rnk]

In D13549#267790, @klimek wrote:

We're waiting for Reid to find somebody who is good at reviewing this in detail.
Sorry it takes a while, so far we don't have enough trusted Windows experts in the community :(

Oops, I didn't know that. Zach knows C#, but I don't think any of us are VSIX experts.

[zturner]

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

tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs

38

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?

42–47

This would all go away

56

This becomes return new StandardValuesCollection(Enum.GetValues(typeof(Style)))

61–64

This becomes sourceType == typeof(Style) || base.CanConvertFrom(context, sourceType)

69–73

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)

92

Now you can return an enum here instead of a string

115

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?

136–143

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.

331–347

Maybe you could use properties instead of functions.

[curdeius]

Hi Zachary, just to answer your comments. I have done it on purpose not to use enum, because clang-format style can be actually a JSON string, e.g. {BasedOnStyle: "LLVM", IndentWidth: 4}, so it wouldn't translate into an enum (to my knowledge at least). Besides, I would like to have the possibility not to add a new enum value if there is a new style in clang-format.
Please correct me if I'm wrong about enums.

tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs

115

Well, in CanConvertFrom, the sourceType parameter is a Type and not the value given by the user, so no, we cannot do this.

[zturner]

In D13549#268701, @curdeius wrote:

Hi Zachary, just to answer your comments. I have done it on purpose not to use enum, because clang-format style can be actually a JSON string, e.g. {BasedOnStyle: "LLVM", IndentWidth: 4}, so it wouldn't translate into an enum (to my knowledge at least). Besides, I would like to have the possibility not to add a new enum value if there is a new style in clang-format.
Please correct me if I'm wrong about enums.

Where is the code in the CL that handles extracting that value from a JSON string? Because it looks like you're just building an array list of the trivial non-JSON style names, so why couldn't that be an enum? I don't know mucha bout clang-format, but looking at the comments it seems like the JSON only comes into play when you're reading a .clang-format file, and in that case the value of the enum would be Style.File (until you finish reading that, at which case you set it to Style.LLVM etc).

I'm also not sure what advantage you get by not having to add an enum if there is a new style? You have to change the code either way, and by not using an enum you are likely to miss some of the places in the code that manipulate or process the style since you're bypassing the type system.

[curdeius]

Where is the code in the CL that handles extracting that value from a JSON string? Because it looks like you're just building an array list of the trivial non-JSON style names, so why couldn't that be an enum? I don't know mucha bout clang-format, but looking at the comments it seems like the JSON only comes into play when you're reading a .clang-format file, and in that case the value of the enum would be Style.File (until you finish reading that, at which case you set it to Style.LLVM etc).

The Style option is actually passed to clang-format -style parameter and it's clang-format that parses it. It can be a predefined type (one of: 'none', 'file', 'LLVM', etc.) or a JSON (or JSON-like?) string.

I'm also not sure what advantage you get by not having to add an enum if there is a new style? You have to change the code either way, and by not using an enum you are likely to miss some of the places in the code that manipulate or process the style since you're bypassing the type system.

I wanted to say that it would be better if the user didn't have to wait until the VSIX plugins gets updated after a new style is added to clang-format.

[zturner]

Oh I see. Makes sense then.

[hans]

This is now part of the latest snapshot at http://llvm.org/builds/
Seems to work :-)

I had to dig around a bit to figure out where these settings are actually exposed. Should we mention that in the documentation somewhere? Actually, do we even have any documentation for this plugin?

tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs

 
 using Microsoft.VisualStudio.Editor;
 using Microsoft.VisualStudio.Shell;
 using Microsoft.VisualStudio.Shell.Interop;
 using Microsoft.VisualStudio.Text;
 using Microsoft.VisualStudio.Text.Editor;
 using Microsoft.VisualStudio.TextManager.Interop;
 using System;
+using System.Collections;
 using System.ComponentModel;
 using System.ComponentModel.Design;
 using System.IO;
-using System.Reflection;
 using System.Runtime.InteropServices;
 using System.Xml.Linq;
 
 namespace LLVM.ClangFormat
 {
     [ClassInterface(ClassInterfaceType.AutoDual)]
     [CLSCompliant(false), ComVisible(true)]
     public class OptionPageGrid : DialogPage
     {
-        private string style = "File";
+        private string assumeFilename = "";
+        private string fallbackStyle = "LLVM";
+        private bool sortIncludes = false;
+        private string style = "file";
+
+        public class StyleConverter : TypeConverter
+        {
+            protected ArrayList values;
+            public StyleConverter()
+            {
+                // Initializes the standard values list with defaults.
+                values = new ArrayList(new string[] { "file", "Chromium", "Google", "LLVM", "Mozilla", "WebKit" });
+            }
+
+            public override bool GetStandardValuesSupported(ITypeDescriptorContext context)
+            {
+                return true;
+            }
+
+            public override StandardValuesCollection GetStandardValues(ITypeDescriptorContext context)
+            {
+                return new StandardValuesCollection(values);
+            }
+
+            public override bool CanConvertFrom(ITypeDescriptorContext context, Type sourceType)
+            {
+                if (sourceType == typeof(string))
+                    return true;
+
+                return base.CanConvertFrom(context, sourceType);
+            }
+
+            public override object ConvertFrom(ITypeDescriptorContext context, System.Globalization.CultureInfo culture, object value)
+            {
+                string s = value as string;
+                if (s == null)
+                    return base.ConvertFrom(context, culture, value);
+
+                return value;
+            }
+        }
 
         [Category("LLVM/Clang")]
         [DisplayName("Style")]
         [Description("Coding style, currently supports:\n" +
-                     "  - Predefined styles ('LLVM', 'Google', 'Chromium', 'Mozilla').\n" +
-                     "  - 'File' to search for a YAML .clang-format or _clang-format\n" +
+                     "  - Predefined styles ('LLVM', 'Google', 'Chromium', 'Mozilla', 'WebKit').\n" +
+                     "  - 'file' to search for a YAML .clang-format or _clang-format\n" +
                      "    configuration file.\n" +
                      "  - A YAML configuration snippet.\n\n" +
                      "'File':\n" +
                      "  Searches for a .clang-format or _clang-format configuration file\n" +
                      "  in the source file's directory and its parents.\n\n" +
                      "YAML configuration snippet:\n" +
                      "  The content of a .clang-format configuration file, as string.\n" +
                      "  Example: '{BasedOnStyle: \"LLVM\", IndentWidth: 8}'\n\n" +
                      "See also: http://clang.llvm.org/docs/ClangFormatStyleOptions.html.")]
+        [TypeConverter(typeof(StyleConverter))]
         public string Style
         {
             get { return style; }
             set { style = value; }
         }
+
+        public sealed class FilenameConverter : TypeConverter
+        {
+            public override bool CanConvertFrom(ITypeDescriptorContext context, Type sourceType)
+            {
+                if (sourceType == typeof(string))
+                    return true;
+
+                return base.CanConvertFrom(context, sourceType);
+            }
+
+            public override object ConvertFrom(ITypeDescriptorContext context, System.Globalization.CultureInfo culture, object value)
+            {
+                string s = value as string;
+                if (s == null)
+                    return base.ConvertFrom(context, culture, value);
+
+                // 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.
+                if (s.IndexOf('\"') != -1)
+                    throw new NotSupportedException("Filename cannot contain quotes");
+
+                return value;
+            }
+        }
+
+        [Category("LLVM/Clang")]
+        [DisplayName("Assume Filename")]
+        [Description("When reading from stdin, clang-format assumes this " +
+                     "filename to look for a style config file (with 'file' style) " +
+                     "and to determine the language.")]
+        [TypeConverter(typeof(FilenameConverter))]
+        public string AssumeFilename
+        {
+            get { return assumeFilename; }
+            set { assumeFilename = value; }
+        }
+
+        public sealed class FallbackStyleConverter : StyleConverter
+        {
+            public FallbackStyleConverter()
+            {
+                // Add "none" to the list of styles.
+                values.Insert(0, "none");
+            }
+        }
+
+        [Category("LLVM/Clang")]
+        [DisplayName("Fallback Style")]
+        [Description("The name of the predefined style used as a fallback in case clang-format " +
+                     "is invoked with 'file' style, but can not find the configuration file.\n" +
+                     "Use 'none' fallback style to skip formatting.")]
+        [TypeConverter(typeof(FallbackStyleConverter))]
+        public string FallbackStyle
+        {
+            get { return fallbackStyle; }
+            set { fallbackStyle = value; }
+        }
+
+        [Category("LLVM/Clang")]
+        [DisplayName("Sort includes")]
+        [Description("Sort touched include lines.\n\n" +
+                     "See also: http://clang.llvm.org/docs/ClangFormat.html.")]
+        public bool SortIncludes
+        {
+            get { return sortIncludes; }
+            set { sortIncludes = value; }
+        }
     }
 
     [PackageRegistration(UseManagedResourcesOnly = true)]
     [InstalledProductRegistration("#110", "#112", "1.0", IconResourceID = 400)]
     [ProvideMenuResource("Menus.ctmenu", 1)]
     [Guid(GuidList.guidClangFormatPkgString)]
     [ProvideOptionPage(typeof(OptionPageGrid), "LLVM/Clang", "ClangFormat", 0, 0, true)]
     public sealed class ClangFormatPackage : Package
public sealed class ClangFormatPackage : Package
                 typeof(ClangFormatPackage).Assembly.Location);
 
             System.Diagnostics.Process process = new System.Diagnostics.Process();
             process.StartInfo.UseShellExecute = false;
             process.StartInfo.FileName = vsixPath + "\\clang-format.exe";
             // Poor man's escaping - this will not work when quotes are already escaped
             // in the input (but we don't need more).
             string style = GetStyle().Replace("\"", "\\\"");
+            string fallbackStyle = GetFallbackStyle().Replace("\"", "\\\"");
             process.StartInfo.Arguments = " -offset " + offset +
                                           " -length " + length +
                                           " -output-replacements-xml " +
-                                          " -style \"" + style + "\"";
+                                          " -style \"" + style + "\"" +
+                                          " -fallback-style \"" + fallbackStyle + "\"";
+            if (GetSortIncludes())
+              process.StartInfo.Arguments += " -sort-includes ";
+            string assumeFilename = GetAssumeFilename();
+            if (!string.IsNullOrEmpty(assumeFilename))
+              process.StartInfo.Arguments += " -assume-filename \"" + assumeFilename + "\"";
             process.StartInfo.CreateNoWindow = true;
             process.StartInfo.RedirectStandardInput = true;
             process.StartInfo.RedirectStandardOutput = true;
             process.StartInfo.RedirectStandardError = true;
             if (path != null)
                 process.StartInfo.WorkingDirectory = path;
             // We have to be careful when communicating via standard input / output,
             // as writes to the buffers will block until they are read from the other side.
public sealed class ClangFormatPackage : Package
         }
 
         private string GetStyle()
         {
             var page = (OptionPageGrid)GetDialogPage(typeof(OptionPageGrid));
             return page.Style;
         }
 
+        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;
+        }
+
         private string GetDocumentParent(IWpfTextView view)
         {
             ITextDocument document;
             if (view.TextBuffer.Properties.TryGetProperty(typeof(ITextDocument), out document))
             {
                 return Directory.GetParent(document.FilePath).ToString();
             }
             return null;
         }
     }
 }

tools/clang-format-vs/ClangFormat/Properties/AssemblyInfo.cs

 //      Major Version
 //      Minor Version 
 //      Build Number
 //      Revision
 //
 // You can specify all the values or you can default the Revision and Build Numbers 
 // by using the '*' as shown below:
 
-[assembly: AssemblyVersion("1.0.0.0")]
-[assembly: AssemblyFileVersion("1.0.0.0")]
+[assembly: AssemblyVersion("1.1.0.0")]
+[assembly: AssemblyFileVersion("1.1.0.0")]

tools/clang-format/ClangFormat.cpp

for (unsigned i = 0, e = Offsets.size(); i != e; ++i) {
     unsigned Offset = Sources.getFileOffset(Start);
     unsigned Length = Sources.getFileOffset(End) - Offset;
     Ranges.push_back(tooling::Range(Offset, Length));
   }
   return false;
 }
 
 static void outputReplacementXML(StringRef Text) {
+  // FIXME: When we sort includes, we need to make sure the stream is correct
+  // utf-8.
   size_t From = 0;
   size_t Index;
-  while ((Index = Text.find_first_of("\n\r", From)) != StringRef::npos) {
+  while ((Index = Text.find_first_of("\n\r<&", From)) != StringRef::npos) {
     llvm::outs() << Text.substr(From, Index - From);
     switch (Text[Index]) {
     case '\n':
       llvm::outs() << "&#10;";
       break;
     case '\r':
       llvm::outs() << "&#13;";
       break;
+    case '<':
+      llvm::outs() << "&lt;";
+      break;
+    case '&':
+      llvm::outs() << "&amp;";
+      break;
     default:
       llvm_unreachable("Unexpected character encountered!");
     }
     From = Index + 1;
   }
   llvm::outs() << Text.substr(From);
 }
 
 static void outputReplacementsXML(const Replacements &Replaces) {
   for (const auto &R : Replaces) {
     outs() << "<replacement "
            << "offset='" << R.getOffset() << "' "
            << "length='" << R.getLength() << "'>";
     outputReplacementXML(R.getReplacementText());
     outs() << "</replacement>\n";
   }
 }