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

[Antonio Maiorano via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL299543: clang-format-vsix: Add "Format on Save" feature 
(authored by amaiorano).

Changed prior to commit:
  https://reviews.llvm.org/D29221?vs=89815=94225#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D29221

Files:
  cfe/trunk/tools/clang-format-vs/ClangFormat/ClangFormat.csproj
  cfe/trunk/tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
  cfe/trunk/tools/clang-format-vs/ClangFormat/RunningDocTableEventsDispatcher.cs
  cfe/trunk/tools/clang-format-vs/ClangFormat/Vsix.cs

Index: cfe/trunk/tools/clang-format-vs/ClangFormat/RunningDocTableEventsDispatcher.cs
===
--- cfe/trunk/tools/clang-format-vs/ClangFormat/RunningDocTableEventsDispatcher.cs
+++ cfe/trunk/tools/clang-format-vs/ClangFormat/RunningDocTableEventsDispatcher.cs
@@ -0,0 +1,79 @@
+using EnvDTE;
+using Microsoft.VisualStudio;
+using Microsoft.VisualStudio.Shell;
+using Microsoft.VisualStudio.Shell.Interop;
+using System.Linq;
+
+namespace LLVM.ClangFormat
+{
+// Exposes event sources for IVsRunningDocTableEvents3 events.
+internal sealed class RunningDocTableEventsDispatcher : IVsRunningDocTableEvents3
+{
+private RunningDocumentTable _runningDocumentTable;
+private DTE _dte;
+
+public delegate void OnBeforeSaveHander(object sender, Document document);
+public event OnBeforeSaveHander BeforeSave;
+
+public RunningDocTableEventsDispatcher(Package package)
+{
+_runningDocumentTable = new RunningDocumentTable(package);
+_runningDocumentTable.Advise(this);
+_dte = (DTE)Package.GetGlobalService(typeof(DTE));
+}
+
+public int OnAfterAttributeChange(uint docCookie, uint grfAttribs)
+{
+return VSConstants.S_OK;
+}
+
+public int OnAfterAttributeChangeEx(uint docCookie, uint grfAttribs, IVsHierarchy pHierOld, uint itemidOld, string pszMkDocumentOld, IVsHierarchy pHierNew, uint itemidNew, string pszMkDocumentNew)
+{
+return VSConstants.S_OK;
+}
+
+public int OnAfterDocumentWindowHide(uint docCookie, IVsWindowFrame pFrame)
+{
+return VSConstants.S_OK;
+}
+
+public int OnAfterFirstDocumentLock(uint docCookie, uint dwRDTLockType, uint dwReadLocksRemaining, uint dwEditLocksRemaining)
+{
+return VSConstants.S_OK;
+}
+
+public int OnAfterSave(uint docCookie)
+{
+return VSConstants.S_OK;
+}
+
+public int OnBeforeDocumentWindowShow(uint docCookie, int fFirstShow, IVsWindowFrame pFrame)
+{
+return VSConstants.S_OK;
+}
+
+public int OnBeforeLastDocumentUnlock(uint docCookie, uint dwRDTLockType, uint dwReadLocksRemaining, uint dwEditLocksRemaining)
+{
+return VSConstants.S_OK;
+}
+
+public int OnBeforeSave(uint docCookie)
+{
+if (BeforeSave != null)
+{
+var document = FindDocumentByCookie(docCookie);
+if (document != null) // Not sure why this happens sometimes
+{
+BeforeSave(this, FindDocumentByCookie(docCookie));
+}
+}
+return VSConstants.S_OK;
+}
+
+private Document FindDocumentByCookie(uint docCookie)
+{
+var documentInfo = _runningDocumentTable.GetDocumentInfo(docCookie);
+return _dte.Documents.Cast().FirstOrDefault(doc => doc.FullName == documentInfo.Moniker);
+}
+}
+}
Index: cfe/trunk/tools/clang-format-vs/ClangFormat/Vsix.cs
===
--- cfe/trunk/tools/clang-format-vs/ClangFormat/Vsix.cs
+++ cfe/trunk/tools/clang-format-vs/ClangFormat/Vsix.cs
@@ -0,0 +1,96 @@
+using EnvDTE;
+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.IO;
+
+namespace LLVM.ClangFormat
+{
+internal sealed class Vsix
+{
+/// 
+/// Returns the currently active view if it is a IWpfTextView.
+/// 
+public static IWpfTextView GetCurrentView()
+{
+// The SVsTextManager is a service through which we can get the active view.
+var textManager = (IVsTextManager)Package.GetGlobalService(typeof(SVsTextManager));
+IVsTextView textView;
+textManager.GetActiveView(1, null, out textView);
+
+// Now we have the active view as IVsTextView, but the text interfaces we need
+// are in the IWpfTextView.
+return VsToWpfTextView(textView);
+}
+
+public 

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

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D29221#713902, @hans wrote:

> In https://reviews.llvm.org/D29221#698679, @amaiorano wrote:
>
> > Hey guys, any feedback on this? About 1.5 weeks ago, @hans asked @klimek 
> > and @djasper for their opinions on "clang-format" vs "ClangFormat". Thanks!
>
>
> Let's get this committed now and not worry about renaming things.
>
> Do you have commit access, or would you like me to commit it for you?


I've got commit access, so I'll commit this ASAP. Thanks!


https://reviews.llvm.org/D29221



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D29221#688697, @hans wrote:

> >>> My only nit is that I'd prefer "clang-format" instead of "ClangFormat".
> >>> 
> >>> Manuel: the menu options under Tools currently say "Clang Format 
> >>> {Selection,Document}". What do you think about using "clang-format" here 
> >>> instead? That is the name of the tool after all, and I think it also 
> >>> works nicely as a verb.
> >>> 
> >>> I realize the actual extension is called ClangFormat, but maybe there 
> >>> were reasons for that, or we have to live with it?
> >> 
> >> I would also like clang-format in there rather than ClangFormat. One thing 
> >> to validate is whether this change would mean people would lose their 
> >> changes to the defaults in the configuration panel. I'll run some tests 
> >> and see if this is indeed the case. Maybe there's a way to keep the 
> >> internal name the same for config purposes, but change the displayed name 
> >> instead. Will get back to you.
> > 
> > Okay, I looked into it, and the good news is that changing the name of the 
> > category from ClangFormat to clang-format doesn't reset the previously 
> > saved changes made in the options menu. I also changed the menu item names. 
> > Here's what both would look like:
> > 
> > F3119242: pasted_file 
> > 
> > F3119244: pasted_file 
> > 
> > Now having said that, I'm not sure if this is the best change because of a 
> > few things:
> > 
> > 1. The extension's name itself is ClangFormat, which was recently added to 
> > the Visual Studio market place here: 
> > https://marketplace.visualstudio.com/items?itemName=HansWennborg.ClangFormat
> >  . Consequently, the extension appears with this name in the Visual Studio 
> > extensions dialog: F3119249: pasted_file 
> > . I don't think it would be easy to 
> > change this. You cannot really take down an extension page on the 
> > marketplace once it's there; you'd have to document that it has been 
> > deprecated and provide a link to a new page. When searching for it in the 
> > Extensions dialog in VS, though, both the old and new extension would show 
> > up.
> > 2. Although the name of the executable is indeed "clang-format", the 
> > documentation here  uses the 
> > name ClangFormat: F3119251: pasted_file 
> > 
> >   So I leave it up to you whether you really want this change or not. We 
> > can also decide later rather than fold it into this change.
>
> Personally I think we should refer to the tool and the action of formatting 
> as "clang-format" as much as possible. It's unfortunate we can't rename the 
> extension, but maybe that slight inconsistency isn't the end of the world.
>
> Manuel, Daniel: I'd like to hear your opinions here.


Hey guys, any feedback on this? About 1.5 weeks ago, @hans asked @klimek and 
@djasper for their opinions on "clang-format" vs "ClangFormat". Thanks!


https://reviews.llvm.org/D29221



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D29221#687438, @amaiorano wrote:

> In https://reviews.llvm.org/D29221#687425, @hans wrote:
>
> > In https://reviews.llvm.org/D29221#686865, @amaiorano wrote:
> >
> > > Made changes based on @hans 's feedback.
> > >
> > > I looked again at the categories, and I think it makes sense with my 
> > > changes. Here's an updated screenshot that shows what the options menu in 
> > > Visual Studio looks like with these changes with the top-level category 
> > > name ("LLVM/Clang") and page name ("Clang Format") highlighted, as well 
> > > as the category names for the page items ("Format Options" and "Format On 
> > > Save") highlighted:
> > >
> > > F3117958: pasted_file 
> > >
> > > It would be nice if "Format On Save" was _below_ "Format Options", but 
> > > Visual Studio always orders the page categories in alphabetical order, so 
> > > this is pretty standard.
> >
> >
> > The screenshot looks good.
> >
> > My only nit is that I'd prefer "clang-format" instead of "ClangFormat".
> >
> > Manuel: the menu options under Tools currently say "Clang Format 
> > {Selection,Document}". What do you think about using "clang-format" here 
> > instead? That is the name of the tool after all, and I think it also works 
> > nicely as a verb.
> >
> > I realize the actual extension is called ClangFormat, but maybe there were 
> > reasons for that, or we have to live with it?
>
>
> I would also like clang-format in there rather than ClangFormat. One thing to 
> validate is whether this change would mean people would lose their changes to 
> the defaults in the configuration panel. I'll run some tests and see if this 
> is indeed the case. Maybe there's a way to keep the internal name the same 
> for config purposes, but change the displayed name instead. Will get back to 
> you.


Okay, I looked into it, and the good news is that changing the name of the 
category from ClangFormat to clang-format doesn't reset the previously saved 
changes made in the options menu. I also changed the menu item names. Here's 
what both would look like:

F3119242: pasted_file 

F3119244: pasted_file 

Now having said that, I'm not sure if this is the best change because of a few 
things:

1. The extension's name itself is ClangFormat, which was recently added to the 
Visual Studio market place here: 
https://marketplace.visualstudio.com/items?itemName=HansWennborg.ClangFormat . 
Consequently, the extension appears with this name in the Visual Studio 
extensions dialog: F3119249: pasted_file . I 
don't think it would be easy to change this. You cannot really take down an 
extension page on the marketplace once it's there; you'd have to document that 
it has been deprecated and provide a link to a new page. When searching for it 
in the Extensions dialog in VS, though, both the old and new extension would 
show up.

2. Although the name of the executable is indeed "clang-format", the 
documentation here  uses the name 
ClangFormat: F3119251: pasted_file 

So I leave it up to you whether you really want this change or not. We can also 
decide later rather than fold it into this change.


https://reviews.llvm.org/D29221



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D29221#687425, @hans wrote:

> In https://reviews.llvm.org/D29221#686865, @amaiorano wrote:
>
> > Made changes based on @hans 's feedback.
> >
> > I looked again at the categories, and I think it makes sense with my 
> > changes. Here's an updated screenshot that shows what the options menu in 
> > Visual Studio looks like with these changes with the top-level category 
> > name ("LLVM/Clang") and page name ("Clang Format") highlighted, as well as 
> > the category names for the page items ("Format Options" and "Format On 
> > Save") highlighted:
> >
> > F3117958: pasted_file 
> >
> > It would be nice if "Format On Save" was _below_ "Format Options", but 
> > Visual Studio always orders the page categories in alphabetical order, so 
> > this is pretty standard.
>
>
> The screenshot looks good.
>
> My only nit is that I'd prefer "clang-format" instead of "ClangFormat".
>
> Manuel: the menu options under Tools currently say "Clang Format 
> {Selection,Document}". What do you think about using "clang-format" here 
> instead? That is the name of the tool after all, and I think it also works 
> nicely as a verb.
>
> I realize the actual extension is called ClangFormat, but maybe there were 
> reasons for that, or we have to live with it?


I would also like clang-format in there rather than ClangFormat. One thing to 
validate is whether this change would mean people would lose their changes to 
the defaults in the configuration panel. I'll run some tests and see if this is 
indeed the case. Maybe there's a way to keep the internal name the same for 
config purposes, but change the displayed name instead. Will get back to you.


https://reviews.llvm.org/D29221



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano updated this revision to Diff 89815.
amaiorano added a comment.

Made changes based on @hans 's feedback.

I looked again at the categories, and I think it makes sense with my changes. 
Here's an updated screenshot that shows what the options menu in Visual Studio 
looks like with these changes with the top-level category name ("LLVM/Clang") 
and page name ("Clang Format") highlighted, as well as the category names for 
the page items ("Format Options" and "Format On Save") highlighted:

F3117958: pasted_file 

It would be nice if "Format On Save" was _below_ "Format Options", but Visual 
Studio always orders the page categories in alphabetical order, so this is 
pretty standard.


https://reviews.llvm.org/D29221

Files:
  tools/clang-format-vs/ClangFormat/ClangFormat.csproj
  tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
  tools/clang-format-vs/ClangFormat/RunningDocTableEventsDispatcher.cs
  tools/clang-format-vs/ClangFormat/Vsix.cs

Index: tools/clang-format-vs/ClangFormat/Vsix.cs
===
--- /dev/null
+++ tools/clang-format-vs/ClangFormat/Vsix.cs
@@ -0,0 +1,96 @@
+using EnvDTE;
+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.IO;
+
+namespace LLVM.ClangFormat
+{
+internal sealed class Vsix
+{
+/// 
+/// Returns the currently active view if it is a IWpfTextView.
+/// 
+public static IWpfTextView GetCurrentView()
+{
+// The SVsTextManager is a service through which we can get the active view.
+var textManager = (IVsTextManager)Package.GetGlobalService(typeof(SVsTextManager));
+IVsTextView textView;
+textManager.GetActiveView(1, null, out textView);
+
+// Now we have the active view as IVsTextView, but the text interfaces we need
+// are in the IWpfTextView.
+return VsToWpfTextView(textView);
+}
+
+public static bool IsDocumentDirty(Document document)
+{
+var textView = GetDocumentView(document);
+var textDocument = GetTextDocument(textView);
+return textDocument?.IsDirty == true;
+}
+
+public static IWpfTextView GetDocumentView(Document document)
+{
+var textView = GetVsTextViewFrompPath(document.FullName);
+return VsToWpfTextView(textView);
+}
+
+public static IWpfTextView VsToWpfTextView(IVsTextView textView)
+{
+var userData = (IVsUserData)textView;
+if (userData == null)
+return null;
+Guid guidWpfViewHost = DefGuidList.guidIWpfTextViewHost;
+object host;
+userData.GetData(ref guidWpfViewHost, out host);
+return ((IWpfTextViewHost)host).TextView;
+}
+
+public static IVsTextView GetVsTextViewFrompPath(string filePath)
+{
+// From http://stackoverflow.com/a/2427368/4039972
+var dte2 = (EnvDTE80.DTE2)Package.GetGlobalService(typeof(SDTE));
+var sp = (Microsoft.VisualStudio.OLE.Interop.IServiceProvider)dte2;
+var serviceProvider = new Microsoft.VisualStudio.Shell.ServiceProvider(sp);
+
+IVsUIHierarchy uiHierarchy;
+uint itemID;
+IVsWindowFrame windowFrame;
+if (VsShellUtilities.IsDocumentOpen(serviceProvider, filePath, Guid.Empty,
+out uiHierarchy, out itemID, out windowFrame))
+{
+// Get the IVsTextView from the windowFrame.
+return VsShellUtilities.GetTextView(windowFrame);
+}
+return null;
+}
+
+public static ITextDocument GetTextDocument(IWpfTextView view)
+{
+ITextDocument document;
+if (view != null && view.TextBuffer.Properties.TryGetProperty(typeof(ITextDocument), out document))
+return document;
+return null;
+}
+
+public static string GetDocumentParent(IWpfTextView view)
+{
+ITextDocument document = GetTextDocument(view);
+if (document != null)
+{
+return Directory.GetParent(document.FilePath).ToString();
+}
+return null;
+}
+
+public static string GetDocumentPath(IWpfTextView view)
+{
+return GetTextDocument(view)?.FilePath;
+}
+}
+}
Index: tools/clang-format-vs/ClangFormat/RunningDocTableEventsDispatcher.cs
===
--- /dev/null
+++ tools/clang-format-vs/ClangFormat/RunningDocTableEventsDispatcher.cs
@@ -0,0 +1,79 @@
+using 

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

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added inline comments.



Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:276
+
+if (Vsix.IsDocumentDirty(document))
+{

hans wrote:
> Perhaps return early if `!Vsix.IsDocumentDirty(document)` instead?
Yes, will do.


https://reviews.llvm.org/D29221



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added inline comments.



Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:39
 private string style = "file";
+private bool formatOnSaveEnabled = false;
+private string formatOnSaveFileExtensions =

hans wrote:
> Perhaps just `formatOnSave`, similar to `sortIncludes` above?
Will do.



Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:46
+{
+// Use MemberwiseClone to copy value types
+var clone = (OptionPageGrid)MemberwiseClone();

hans wrote:
> Ultra nit: end the sentence with a period.
Will do.



Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:88
 
-[Category("LLVM/Clang")]
+[Category("Format Options")]
 [DisplayName("Style")]

hans wrote:
> What do you think about using "clang-format" for the category name?
So if you take a look at the screenshot I posted with the original diff, you'll 
see how these categories show up:
https://reviews.llvm.org/file/data/cuztal767fqmcy2k7kkv/PHID-FILE-xcoqfwj3o2tpwbabbak5/pasted_file

"LLVM/Clang" is the main option menu name, and was always there. I figure the 
idea is that if we write other extensions, they'd all fall under this heading. 
Personally I'd like for it to be "clang-format" or "Clang Format".

As part of my change, I grouped the options that were there before under 
"Format Options", which is what they are (arguments to clang-format), and my 
new options under "Format On Save".



Comment at: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs:178
+
+[Category("Format On Save")]
+[DisplayName("Enable")]

hans wrote:
> Does this mean the "FormatOnSave" is not nested under the same category as 
> the other clang-format options?
See my answer to the comment on line 88.


https://reviews.llvm.org/D29221



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

Hello again. Just wondering if anyone could take a look at this? I updated the 
patch a week ago :) Thanks!


https://reviews.llvm.org/D29221



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano updated this revision to Diff 88000.
amaiorano added a comment.

- Renamed VsixUtils to Vsix and TypeConverterUtils to TypeConversion
- Removed format on save mode

The change is simpler now without the "mode" (current or all documents). Now 
I'm wondering whether I should rename the feature to Format _Document_ On Save, 
and reflect this name at least in the options grid, if not the related variable 
names.


https://reviews.llvm.org/D29221

Files:
  tools/clang-format-vs/ClangFormat/ClangFormat.csproj
  tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
  tools/clang-format-vs/ClangFormat/RunningDocTableEventsDispatcher.cs
  tools/clang-format-vs/ClangFormat/Vsix.cs

Index: tools/clang-format-vs/ClangFormat/Vsix.cs
===
--- /dev/null
+++ tools/clang-format-vs/ClangFormat/Vsix.cs
@@ -0,0 +1,96 @@
+using EnvDTE;
+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.IO;
+
+namespace LLVM.ClangFormat
+{
+internal sealed class Vsix
+{
+/// 
+/// Returns the currently active view if it is a IWpfTextView.
+/// 
+public static IWpfTextView GetCurrentView()
+{
+// The SVsTextManager is a service through which we can get the active view.
+var textManager = (IVsTextManager)Package.GetGlobalService(typeof(SVsTextManager));
+IVsTextView textView;
+textManager.GetActiveView(1, null, out textView);
+
+// Now we have the active view as IVsTextView, but the text interfaces we need
+// are in the IWpfTextView.
+return VsToWpfTextView(textView);
+}
+
+public static bool IsDocumentDirty(Document document)
+{
+var textView = GetDocumentView(document);
+var textDocument = GetTextDocument(textView);
+return textDocument?.IsDirty == true;
+}
+
+public static IWpfTextView GetDocumentView(Document document)
+{
+var textView = GetVsTextViewFrompPath(document.FullName);
+return VsToWpfTextView(textView);
+}
+
+public static IWpfTextView VsToWpfTextView(IVsTextView textView)
+{
+var userData = (IVsUserData)textView;
+if (userData == null)
+return null;
+Guid guidWpfViewHost = DefGuidList.guidIWpfTextViewHost;
+object host;
+userData.GetData(ref guidWpfViewHost, out host);
+return ((IWpfTextViewHost)host).TextView;
+}
+
+public static IVsTextView GetVsTextViewFrompPath(string filePath)
+{
+// From http://stackoverflow.com/a/2427368/4039972
+var dte2 = (EnvDTE80.DTE2)Package.GetGlobalService(typeof(SDTE));
+var sp = (Microsoft.VisualStudio.OLE.Interop.IServiceProvider)dte2;
+var serviceProvider = new Microsoft.VisualStudio.Shell.ServiceProvider(sp);
+
+IVsUIHierarchy uiHierarchy;
+uint itemID;
+IVsWindowFrame windowFrame;
+if (VsShellUtilities.IsDocumentOpen(serviceProvider, filePath, Guid.Empty,
+out uiHierarchy, out itemID, out windowFrame))
+{
+// Get the IVsTextView from the windowFrame.
+return VsShellUtilities.GetTextView(windowFrame);
+}
+return null;
+}
+
+public static ITextDocument GetTextDocument(IWpfTextView view)
+{
+ITextDocument document;
+if (view != null && view.TextBuffer.Properties.TryGetProperty(typeof(ITextDocument), out document))
+return document;
+return null;
+}
+
+public static string GetDocumentParent(IWpfTextView view)
+{
+ITextDocument document = GetTextDocument(view);
+if (document != null)
+{
+return Directory.GetParent(document.FilePath).ToString();
+}
+return null;
+}
+
+public static string GetDocumentPath(IWpfTextView view)
+{
+return GetTextDocument(view)?.FilePath;
+}
+}
+}
Index: tools/clang-format-vs/ClangFormat/RunningDocTableEventsDispatcher.cs
===
--- /dev/null
+++ tools/clang-format-vs/ClangFormat/RunningDocTableEventsDispatcher.cs
@@ -0,0 +1,79 @@
+using EnvDTE;
+using Microsoft.VisualStudio;
+using Microsoft.VisualStudio.Shell;
+using Microsoft.VisualStudio.Shell.Interop;
+using System.Linq;
+
+namespace LLVM.ClangFormat
+{
+// Exposes event sources for IVsRunningDocTableEvents3 events.
+internal sealed class RunningDocTableEventsDispatcher : 

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

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D29221#668867, @klimek wrote:

> +hans
>
> +1 to "format only current document but save all" not making much sense :)


Yes, I've been using this version for a little while now, and it's not useful 
to have the current document version. I'll remove it, which will simplify the 
code as well.

I'll wait a bit more for comments from others before uploading a new version.

Thanks!




Comment at: tools/clang-format-vs/ClangFormat/TypeConverterUtils.cs:7
+{
+public sealed class TypeConverterUtils
+{

klimek wrote:
> Perhaps name this "TypeConversion" or something - I find "utils" as part of a 
> name to not add anything, and it tends to accumulate cruft :)
Sounds great, will do that.



Comment at: tools/clang-format-vs/ClangFormat/VsixUtils.cs:13
+{
+internal sealed class VsixUtils
+{

klimek wrote:
> Same here, just call it Vsix? (context seems clear enough)
Perfect, will also make this change.


https://reviews.llvm.org/D29221



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

Hello! Just a small ping to see if anyone has taken a look at this change? I 
fully understand that everyone's really busy, but perhaps you can recommend 
another reviewer? Or if you're presently giving this change a whirl, just let 
me know! Cheers :)


https://reviews.llvm.org/D29221



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

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

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano created this revision.

This change adds a feature to the clang-format VS extension that optionally 
enables the automatic formatting of documents when saving. Since developers 
always need to save their files, this eases the workflow of making sure source 
files are properly formatted. This feature exists in other IDEs, notably in 
Eclipse ; furthermore, there is a 
VS extension that provides this functionality 

 for generic formatters, so there is definitely a precedent for this type of 
feature.

Although this patch works, I know it's quite big and potentially contentious, 
so I'd like to start a discussion about it here. I would appreciate it if you 
could try it out locally for a couple days to see how it feels. Personally, 
I've been using it and like how it eases my workflow.

Here's a screenshot of what the options grid looks like now by default:
F3029838: pasted_file 

Things to note:

- I renamed the category "LLVM/Clang" to "Format Options" and added one to 
group the "Format on Save" options.

- The "File extensions" field is needed to filter out non-source code documents 
that are modified in VS, but that we don't want to format with clang-format. 
The list of extensions includes all possible C/C++ file extensions (I hope), 
and the list of extensions presently supported by clang-format.

- The "Mode" field allows you to select "All Documents" or "Current Document", 
which effectively scopes the formatting on save to all modified documents or 
current modified document respectively. I'm still on the fence on whether to 
even bother offering this mode, especially since "Current Document" can be 
weird since you could modify multiple files, then "save all" (which happens 
when you trigger a build), and find that only the current document gets 
formatted. Perhaps it's just better to only support "All Documents" and remove 
this field.

Other things I'd like to call out in this change:

- When formatting on save, we ignore the FallbackStyle in the user's options 
and set it to "none". This is to make sure we only format files based on Style: 
for e.g., if Style if "file", we only format on save when a .clang-format file 
is found; but we don't format files in projects that have no .clang-format. I 
think this makes sense since "format on save" is a global option, and users 
would be surprised to find it formatting files in projects that don't have a 
.clang-format file. The description of the "Enable" field in the options dialog 
explains this.

- I split out some helper functions into a VsixUtils class and file, and added 
new utility functions for format on save. I also added 2 new files with more 
related helpers.

- I added the attribute [ProvideAutoLoad(UIContextGuids80.SolutionExists)] to 
make the extension load as soon as a solution is loaded, rather than have it 
load lazily upon the first format line/document call. This is required so that 
I can register for the BeforeSave callback right away, otherwise formatting on 
save would not work until the user first explicitly formats (via menu or 
keyboard shortcut).

- I did some refactoring around OptionsPageGrid so that rather than accessing 
it directly in RunClangFormat(), we pass it down as args. This allows me to 
override user options, specifically the FallbackStyle, when formatting on save. 
(line 320)

Thanks, and I looked forward to your feedback!


https://reviews.llvm.org/D29221

Files:
  tools/clang-format-vs/ClangFormat/ClangFormat.csproj
  tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
  tools/clang-format-vs/ClangFormat/RunningDocTableEventsDispatcher.cs
  tools/clang-format-vs/ClangFormat/TypeConverterUtils.cs
  tools/clang-format-vs/ClangFormat/VsixUtils.cs

Index: tools/clang-format-vs/ClangFormat/VsixUtils.cs
===
--- /dev/null
+++ tools/clang-format-vs/ClangFormat/VsixUtils.cs
@@ -0,0 +1,96 @@
+using EnvDTE;
+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.IO;
+
+namespace LLVM.ClangFormat
+{
+internal sealed class VsixUtils
+{
+/// 
+/// Returns the currently active view if it is a IWpfTextView.
+/// 
+public static IWpfTextView GetCurrentView()
+{
+// The SVsTextManager is a service through which we can get the active view.
+var textManager = (IVsTextManager)Package.GetGlobalService(typeof(SVsTextManager));
+IVsTextView textView;
+textManager.GetActiveView(1, null, out textView);
+
+// Now we have the active view as IVsTextView, but the text 

[PATCH] D28983: clang-format: remove tests that assume no config file will be found as this is not always the case

[Antonio Maiorano via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL292787: clang-format: remove tests that assume no config 
file will be found as this is… (authored by amaiorano).

Changed prior to commit:
  https://reviews.llvm.org/D28983?vs=85247=85366#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D28983

Files:
  cfe/trunk/test/Format/style-on-command-line.cpp


Index: cfe/trunk/test/Format/style-on-command-line.cpp
===
--- cfe/trunk/test/Format/style-on-command-line.cpp
+++ cfe/trunk/test/Format/style-on-command-line.cpp
@@ -13,16 +13,11 @@
 // RUN: clang-format -style="{BasedOnStyle: WebKit, PointerBindsToType: 
false}" %s | FileCheck -strict-whitespace -check-prefix=CHECK9 %s
 
 // Fallback style tests
-// RUN: rm %T/_clang-format
-// Test no config file found, WebKit fallback style is applied
-// RUN: clang-format -style=file -fallback-style=WebKit 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK10 %s
-// Test no config file and fallback style "none", no formatting is applied
-// RUN: clang-format -style=file -fallback-style=none 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK11 %s
 // Test config file with no based style, and fallback style "none", formatting 
is applied
 // RUN: printf "IndentWidth: 6\n" > %T/_clang-format
-// RUN: clang-format -style=file -fallback-style=none 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK12 %s
+// RUN: clang-format -style=file -fallback-style=none 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK10 %s
 // Test yaml with no based style, and fallback style "none", LLVM formatting 
applied
-// RUN: clang-format -style="{IndentWidth: 7}" -fallback-style=none %s | 
FileCheck -strict-whitespace -check-prefix=CHECK13 %s
+// RUN: clang-format -style="{IndentWidth: 7}" -fallback-style=none %s | 
FileCheck -strict-whitespace -check-prefix=CHECK11 %s
 
 void f() {
 // CHECK1: {{^int\* i;$}}
@@ -35,10 +30,8 @@
 // CHECK7: {{^  int\* i;$}}
 // CHECK8: {{^  int\* i;$}}
 // CHECK9: {{^int \*i;$}}
-// CHECK10: {{^int\* i;$}}
-// CHECK11: {{^int\*i;$}}
-// CHECK12: {{^  int \*i;$}}
-// CHECK13: {{^   int \*i;$}}
+// CHECK10: {{^  int \*i;$}}
+// CHECK11: {{^   int \*i;$}}
 int*i;
 int j;
 }


Index: cfe/trunk/test/Format/style-on-command-line.cpp
===
--- cfe/trunk/test/Format/style-on-command-line.cpp
+++ cfe/trunk/test/Format/style-on-command-line.cpp
@@ -13,16 +13,11 @@
 // RUN: clang-format -style="{BasedOnStyle: WebKit, PointerBindsToType: false}" %s | FileCheck -strict-whitespace -check-prefix=CHECK9 %s
 
 // Fallback style tests
-// RUN: rm %T/_clang-format
-// Test no config file found, WebKit fallback style is applied
-// RUN: clang-format -style=file -fallback-style=WebKit -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK10 %s
-// Test no config file and fallback style "none", no formatting is applied
-// RUN: clang-format -style=file -fallback-style=none -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK11 %s
 // Test config file with no based style, and fallback style "none", formatting is applied
 // RUN: printf "IndentWidth: 6\n" > %T/_clang-format
-// RUN: clang-format -style=file -fallback-style=none -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK12 %s
+// RUN: clang-format -style=file -fallback-style=none -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK10 %s
 // Test yaml with no based style, and fallback style "none", LLVM formatting applied
-// RUN: clang-format -style="{IndentWidth: 7}" -fallback-style=none %s | FileCheck -strict-whitespace -check-prefix=CHECK13 %s
+// RUN: clang-format -style="{IndentWidth: 7}" -fallback-style=none %s | FileCheck -strict-whitespace -check-prefix=CHECK11 %s
 
 void f() {
 // CHECK1: {{^int\* i;$}}
@@ -35,10 +30,8 @@
 // CHECK7: {{^  int\* i;$}}
 // CHECK8: {{^  int\* i;$}}
 // CHECK9: {{^int \*i;$}}
-// CHECK10: {{^int\* i;$}}
-// CHECK11: {{^int\*i;$}}
-// CHECK12: {{^  int \*i;$}}
-// CHECK13: {{^   int \*i;$}}
+// CHECK10: {{^  int \*i;$}}
+// CHECK11: {{^   int \*i;$}}
 int*i;
 int j;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27440: clang-format-vsix: fail when clang-format outputs to stderr

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano abandoned this revision.
amaiorano added a comment.

This change is no longer needed since clang-format now returns failure status 
(non-zero) whenever it writes to stderr (e.g. on parse error) since 
https://llvm.org/svn/llvm-project/cfe/trunk@292174 
(https://reviews.llvm.org/D28081)


https://reviews.llvm.org/D27440



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28983: clang-format: remove tests that assume no config file will be found as this is not always the case

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano created this revision.
Herald added a subscriber: klimek.

These tests fail for developers who place their build directories under the
llvm root directory because llvm's own .clang-format file will be found.
Anyway these cases are covered by FormatStyle.GetStyleOfFile tests
(FormatTest.cpp).


https://reviews.llvm.org/D28983

Files:
  test/Format/style-on-command-line.cpp


Index: test/Format/style-on-command-line.cpp
===
--- test/Format/style-on-command-line.cpp
+++ test/Format/style-on-command-line.cpp
@@ -13,16 +13,11 @@
 // RUN: clang-format -style="{BasedOnStyle: WebKit, PointerBindsToType: 
false}" %s | FileCheck -strict-whitespace -check-prefix=CHECK9 %s
 
 // Fallback style tests
-// RUN: rm %T/_clang-format
-// Test no config file found, WebKit fallback style is applied
-// RUN: clang-format -style=file -fallback-style=WebKit 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK10 %s
-// Test no config file and fallback style "none", no formatting is applied
-// RUN: clang-format -style=file -fallback-style=none 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK11 %s
 // Test config file with no based style, and fallback style "none", formatting 
is applied
 // RUN: printf "IndentWidth: 6\n" > %T/_clang-format
-// RUN: clang-format -style=file -fallback-style=none 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK12 %s
+// RUN: clang-format -style=file -fallback-style=none 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK10 %s
 // Test yaml with no based style, and fallback style "none", LLVM formatting 
applied
-// RUN: clang-format -style="{IndentWidth: 7}" -fallback-style=none %s | 
FileCheck -strict-whitespace -check-prefix=CHECK13 %s
+// RUN: clang-format -style="{IndentWidth: 7}" -fallback-style=none %s | 
FileCheck -strict-whitespace -check-prefix=CHECK11 %s
 
 void f() {
 // CHECK1: {{^int\* i;$}}
@@ -35,10 +30,8 @@
 // CHECK7: {{^  int\* i;$}}
 // CHECK8: {{^  int\* i;$}}
 // CHECK9: {{^int \*i;$}}
-// CHECK10: {{^int\* i;$}}
-// CHECK11: {{^int\*i;$}}
-// CHECK12: {{^  int \*i;$}}
-// CHECK13: {{^   int \*i;$}}
+// CHECK10: {{^  int \*i;$}}
+// CHECK11: {{^   int \*i;$}}
 int*i;
 int j;
 }


Index: test/Format/style-on-command-line.cpp
===
--- test/Format/style-on-command-line.cpp
+++ test/Format/style-on-command-line.cpp
@@ -13,16 +13,11 @@
 // RUN: clang-format -style="{BasedOnStyle: WebKit, PointerBindsToType: false}" %s | FileCheck -strict-whitespace -check-prefix=CHECK9 %s
 
 // Fallback style tests
-// RUN: rm %T/_clang-format
-// Test no config file found, WebKit fallback style is applied
-// RUN: clang-format -style=file -fallback-style=WebKit -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK10 %s
-// Test no config file and fallback style "none", no formatting is applied
-// RUN: clang-format -style=file -fallback-style=none -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK11 %s
 // Test config file with no based style, and fallback style "none", formatting is applied
 // RUN: printf "IndentWidth: 6\n" > %T/_clang-format
-// RUN: clang-format -style=file -fallback-style=none -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK12 %s
+// RUN: clang-format -style=file -fallback-style=none -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK10 %s
 // Test yaml with no based style, and fallback style "none", LLVM formatting applied
-// RUN: clang-format -style="{IndentWidth: 7}" -fallback-style=none %s | FileCheck -strict-whitespace -check-prefix=CHECK13 %s
+// RUN: clang-format -style="{IndentWidth: 7}" -fallback-style=none %s | FileCheck -strict-whitespace -check-prefix=CHECK11 %s
 
 void f() {
 // CHECK1: {{^int\* i;$}}
@@ -35,10 +30,8 @@
 // CHECK7: {{^  int\* i;$}}
 // CHECK8: {{^  int\* i;$}}
 // CHECK9: {{^int \*i;$}}
-// CHECK10: {{^int\* i;$}}
-// CHECK11: {{^int\*i;$}}
-// CHECK12: {{^  int \*i;$}}
-// CHECK13: {{^   int \*i;$}}
+// CHECK10: {{^  int \*i;$}}
+// CHECK11: {{^   int \*i;$}}
 int*i;
 int j;
 }
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28943: [clang-format] Remove redundant test in style-on-command-line.cpp

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D28943#651489, @ioeric wrote:

> In https://reviews.llvm.org/D28943#651488, @amaiorano wrote:
>
> > In https://reviews.llvm.org/D28943#651470, @ioeric wrote:
> >
> > > @amaiorano: The test itself is correct. It's just that this test failed 
> > > in our internal test. We could've fixed it internally, but the fix would 
> > > be ugly. Since the intended behavior is already covered in the case above 
> > > it, and it's really just checking the default fallback style is LLVM, 
> > > which is not related to the original change, I think it makes sense to 
> > > get rid of the case. Hope you don't mind :)
> >
> >
> > Of course I don't mind :) Why did it fail your internal tests, btw? Just 
> > curious. Was it something I could've detected myself?
>
>
> Probably not... it's just that our default fallback style is "Google" instead 
> of "LLVM".


(I replied the following by email, but I'm not sure where it went... posting it 
here instead)

You mean you build a modified version of clang-format where Style is 
initialized to getGoogleStyle()?

I had wondered whether adding a "defaultStyle" argument might be useful, 
specifically in the case where you want to pass in yaml that simply tweaks the 
default style, but I figured it's not much harder to pass in "BasedOnStyle" in 
the yaml.


Repository:
  rL LLVM

https://reviews.llvm.org/D28943



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28943: [clang-format] Remove redundant test in style-on-command-line.cpp

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D28943#651470, @ioeric wrote:

> @amaiorano: The test itself is correct. It's just that this test failed in 
> our internal test. We could've fixed it internally, but the fix would be 
> ugly. Since the intended behavior is already covered in the case above it, 
> and it's really just checking the default fallback style is LLVM, which is 
> not related to the original change, I think it makes sense to get rid of the 
> case. Hope you don't mind :)


Of course I don't mind :) Why did it fail your internal tests, btw? Just 
curious. Was it something I could've detected myself?


Repository:
  rL LLVM

https://reviews.llvm.org/D28943



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28844: clang-format: fix fallback style set to "none" not formatting

[Antonio Maiorano via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL292562: clang-format: fix fallback style set to "none" not 
always formatting (authored by amaiorano).

Changed prior to commit:
  https://reviews.llvm.org/D28844?vs=84795=85074#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D28844

Files:
  cfe/trunk/lib/Format/Format.cpp
  cfe/trunk/test/Format/style-on-command-line.cpp
  cfe/trunk/unittests/Format/FormatTest.cpp

Index: cfe/trunk/lib/Format/Format.cpp
===
--- cfe/trunk/lib/Format/Format.cpp
+++ cfe/trunk/lib/Format/Format.cpp
@@ -1888,8 +1888,8 @@
 }
 
 llvm::Expected getStyle(StringRef StyleName, StringRef FileName,
- StringRef FallbackStyle, StringRef Code,
- vfs::FileSystem *FS) {
+ StringRef FallbackStyleName,
+ StringRef Code, vfs::FileSystem *FS) {
   if (!FS) {
 FS = vfs::getRealFileSystem().get();
   }
@@ -1903,9 +1903,9 @@
   (Code.contains("\n- (") || Code.contains("\n+ (")))
 Style.Language = FormatStyle::LK_ObjC;
 
-  // FIXME: If FallbackStyle is explicitly "none", format is disabled.
-  if (!getPredefinedStyle(FallbackStyle, Style.Language, ))
-return make_string_error("Invalid fallback style \"" + FallbackStyle.str());
+  FormatStyle FallbackStyle = getNoStyle();
+  if (!getPredefinedStyle(FallbackStyleName, Style.Language, ))
+return make_string_error("Invalid fallback style \"" + FallbackStyleName);
 
   if (StyleName.startswith("{")) {
 // Parse YAML/JSON style from the command line.
@@ -1977,7 +1977,7 @@
 return make_string_error("Configuration file(s) do(es) not support " +
  getLanguageName(Style.Language) + ": " +
  UnsuitableConfigFiles);
-  return Style;
+  return FallbackStyle;
 }
 
 } // namespace format
Index: cfe/trunk/unittests/Format/FormatTest.cpp
===
--- cfe/trunk/unittests/Format/FormatTest.cpp
+++ cfe/trunk/unittests/Format/FormatTest.cpp
@@ -10978,13 +10978,31 @@
   ASSERT_TRUE((bool)Style1);
   ASSERT_EQ(*Style1, getLLVMStyle());
 
-  // Test 2: fallback to default.
+  // Test 2.1: fallback to default.
   ASSERT_TRUE(
   FS.addFile("/b/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style2 = getStyle("file", "/b/test.cpp", "Mozilla", "", );
   ASSERT_TRUE((bool)Style2);
   ASSERT_EQ(*Style2, getMozillaStyle());
 
+  // Test 2.2: no format on 'none' fallback style.
+  Style2 = getStyle("file", "/b/test.cpp", "none", "", );
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getNoStyle());
+
+  // Test 2.3: format if config is found with no based style while fallback is
+  // 'none'.
+  ASSERT_TRUE(FS.addFile("/b/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("IndentWidth: 2")));
+  Style2 = getStyle("file", "/b/test.cpp", "none", "", );
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getLLVMStyle());
+
+  // Test 2.4: format if yaml with no based style, while fallback is 'none'.
+  Style2 = getStyle("{}", "a.h", "none", "", );
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getLLVMStyle());
+
   // Test 3: format file in parent directory.
   ASSERT_TRUE(
   FS.addFile("/c/.clang-format", 0,
Index: cfe/trunk/test/Format/style-on-command-line.cpp
===
--- cfe/trunk/test/Format/style-on-command-line.cpp
+++ cfe/trunk/test/Format/style-on-command-line.cpp
@@ -11,6 +11,21 @@
 // RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s | FileCheck -strict-whitespace -check-prefix=CHECK7 %s
 // RUN: clang-format -style="{BasedOnStyle: LLVM, PointerBindsToType: true}" %s | FileCheck -strict-whitespace -check-prefix=CHECK8 %s
 // RUN: clang-format -style="{BasedOnStyle: WebKit, PointerBindsToType: false}" %s | FileCheck -strict-whitespace -check-prefix=CHECK9 %s
+
+// Fallback style tests
+// RUN: rm %T/_clang-format
+// Test no config file found, WebKit fallback style is applied
+// RUN: clang-format -style=file -fallback-style=WebKit -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK10 %s
+// Test no config file and no fallback style, LLVM style is applied
+// RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK11 %s
+// Test no config file and fallback style "none", no formatting is applied
+// RUN: clang-format -style=file -fallback-style=none -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK12 %s
+// Test config file with no based style, and fallback style "none", formatting is applied
+// RUN: printf "IndentWidth: 6\n" > %T/_clang-format
+// RUN: clang-format -style=file -fallback-style=none 

[PATCH] D28844: clang-format: fix fallback style set to "none" not formatting

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added inline comments.



Comment at: lib/Format/Format.cpp:1906
 
-  // FIXME: If FallbackStyle is explicitly "none", format is disabled.
-  if (!getPredefinedStyle(FallbackStyle, Style.Language, ))
-return make_string_error("Invalid fallback style \"" + 
FallbackStyle.str());
+  FormatStyle FallbackStyle = getNoStyle();
+  if (!getPredefinedStyle(FallbackStyleName, Style.Language, ))

Technically, we don't need to initialize FallbackStyle to getNoStyle() here as 
the call to getPredefinedStyle just below will initialize it. Let me know what 
you would prefer here.



Comment at: test/Format/style-on-command-line.cpp:15
+// RUN: rm %T/_clang-format
+// RUN: clang-format -style=file -fallback-style=WebKit 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK10 %s
+// RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s 2>&1 | 
FileCheck -strict-whitespace -check-prefix=CHECK11 %s

Tests no config file found, WebKit fallback style is applied



Comment at: test/Format/style-on-command-line.cpp:16
+// RUN: clang-format -style=file -fallback-style=WebKit 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK10 %s
+// RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s 2>&1 | 
FileCheck -strict-whitespace -check-prefix=CHECK11 %s
+// RUN: clang-format -style=file -fallback-style=none 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK12 %s

Tests no config file found, no fallback style, LLVM style is applied



Comment at: test/Format/style-on-command-line.cpp:17
+// RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s 2>&1 | 
FileCheck -strict-whitespace -check-prefix=CHECK11 %s
+// RUN: clang-format -style=file -fallback-style=none 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK12 %s
 void f() {

Tests no config file found, fallback style set to "none", no formatting is 
applied


https://reviews.llvm.org/D28844



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28844: clang-format: fix fallback style set to "none" not formatting

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano created this revision.

This change fixes the fact that fallback style set to "none" should not format. 
Without this change, fallback style "none" ends up applying LLVM formatting.


https://reviews.llvm.org/D28844

Files:
  lib/Format/Format.cpp
  test/Format/style-on-command-line.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10978,13 +10978,18 @@
   ASSERT_TRUE((bool)Style1);
   ASSERT_EQ(*Style1, getLLVMStyle());
 
-  // Test 2: fallback to default.
+  // Test 2.1: fallback to default.
   ASSERT_TRUE(
   FS.addFile("/b/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int 
i;")));
   auto Style2 = getStyle("file", "/b/test.cpp", "Mozilla", "", );
   ASSERT_TRUE((bool)Style2);
   ASSERT_EQ(*Style2, getMozillaStyle());
 
+  // Test 2.2: no format on 'none' fallback style.
+  Style2 = getStyle("file", "/b/test.cpp", "none", "", );
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getNoStyle());
+
   // Test 3: format file in parent directory.
   ASSERT_TRUE(
   FS.addFile("/c/.clang-format", 0,
Index: test/Format/style-on-command-line.cpp
===
--- test/Format/style-on-command-line.cpp
+++ test/Format/style-on-command-line.cpp
@@ -11,6 +11,10 @@
 // RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s | FileCheck 
-strict-whitespace -check-prefix=CHECK7 %s
 // RUN: clang-format -style="{BasedOnStyle: LLVM, PointerBindsToType: true}" 
%s | FileCheck -strict-whitespace -check-prefix=CHECK8 %s
 // RUN: clang-format -style="{BasedOnStyle: WebKit, PointerBindsToType: 
false}" %s | FileCheck -strict-whitespace -check-prefix=CHECK9 %s
+// RUN: rm %T/_clang-format
+// RUN: clang-format -style=file -fallback-style=WebKit 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK10 %s
+// RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s 2>&1 | 
FileCheck -strict-whitespace -check-prefix=CHECK11 %s
+// RUN: clang-format -style=file -fallback-style=none 
-assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace 
-check-prefix=CHECK12 %s
 void f() {
 // CHECK1: {{^int\* i;$}}
 // CHECK2: {{^   int \*i;$}}
@@ -22,6 +26,9 @@
 // CHECK7: {{^  int\* i;$}}
 // CHECK8: {{^  int\* i;$}}
 // CHECK9: {{^int \*i;$}}
+// CHECK10: {{^int\* i;$}}
+// CHECK11: {{^  int \*i;$}}
+// CHECK12: {{^int\*i;$}}
 int*i;
 int j;
 }
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -1888,8 +1888,8 @@
 }
 
 llvm::Expected getStyle(StringRef StyleName, StringRef FileName,
- StringRef FallbackStyle, StringRef Code,
- vfs::FileSystem *FS) {
+ StringRef FallbackStyleName,
+ StringRef Code, vfs::FileSystem *FS) {
   if (!FS) {
 FS = vfs::getRealFileSystem().get();
   }
@@ -1903,9 +1903,9 @@
   (Code.contains("\n- (") || Code.contains("\n+ (")))
 Style.Language = FormatStyle::LK_ObjC;
 
-  // FIXME: If FallbackStyle is explicitly "none", format is disabled.
-  if (!getPredefinedStyle(FallbackStyle, Style.Language, ))
-return make_string_error("Invalid fallback style \"" + 
FallbackStyle.str());
+  FormatStyle FallbackStyle = getNoStyle();
+  if (!getPredefinedStyle(FallbackStyleName, Style.Language, ))
+return make_string_error("Invalid fallback style \"" + FallbackStyleName);
 
   if (StyleName.startswith("{")) {
 // Parse YAML/JSON style from the command line.
@@ -1977,7 +1977,7 @@
 return make_string_error("Configuration file(s) do(es) not support " +
  getLanguageName(Style.Language) + ": " +
  UnsuitableConfigFiles);
-  return Style;
+  return FallbackStyle;
 }
 
 } // namespace format


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10978,13 +10978,18 @@
   ASSERT_TRUE((bool)Style1);
   ASSERT_EQ(*Style1, getLLVMStyle());
 
-  // Test 2: fallback to default.
+  // Test 2.1: fallback to default.
   ASSERT_TRUE(
   FS.addFile("/b/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style2 = getStyle("file", "/b/test.cpp", "Mozilla", "", );
   ASSERT_TRUE((bool)Style2);
   ASSERT_EQ(*Style2, getMozillaStyle());
 
+  // Test 2.2: no format on 'none' fallback style.
+  Style2 = getStyle("file", "/b/test.cpp", "none", "", );
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getNoStyle());
+
   // Test 3: format file in parent directory.
   ASSERT_TRUE(
   FS.addFile("/c/.clang-format", 0,
Index: 

[PATCH] D28315: Update tools to use new getStyle API

[Antonio Maiorano via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL292175: Update tools to use new getStyle API (authored by 
amaiorano).

Changed prior to commit:
  https://reviews.llvm.org/D28315?vs=84539=84611#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D28315

Files:
  clang-tools-extra/trunk/change-namespace/ChangeNamespace.cpp
  
clang-tools-extra/trunk/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
  clang-tools-extra/trunk/clang-move/ClangMove.cpp
  clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
  clang-tools-extra/trunk/include-fixer/tool/ClangIncludeFixer.cpp

Index: clang-tools-extra/trunk/include-fixer/tool/ClangIncludeFixer.cpp
===
--- clang-tools-extra/trunk/include-fixer/tool/ClangIncludeFixer.cpp
+++ clang-tools-extra/trunk/include-fixer/tool/ClangIncludeFixer.cpp
@@ -303,10 +303,13 @@
const IncludeFixerContext::HeaderInfo ) {
   return LHS.QualifiedName == RHS.QualifiedName;
 });
-format::FormatStyle InsertStyle =
-format::getStyle("file", Context.getFilePath(), Style);
+auto InsertStyle = format::getStyle("file", Context.getFilePath(), Style);
+if (!InsertStyle) {
+  llvm::errs() << llvm::toString(InsertStyle.takeError()) << "\n";
+  return 1;
+}
 auto Replacements = clang::include_fixer::createIncludeFixerReplacements(
-Code->getBuffer(), Context, InsertStyle,
+Code->getBuffer(), Context, *InsertStyle,
 /*AddQualifiers=*/IsUniqueQualifiedName);
 if (!Replacements) {
   errs() << "Failed to create replacements: "
@@ -378,16 +381,20 @@
   std::vector FixerReplacements;
   for (const auto  : Contexts) {
 StringRef FilePath = Context.getFilePath();
-format::FormatStyle InsertStyle = format::getStyle("file", FilePath, Style);
+auto InsertStyle = format::getStyle("file", FilePath, Style);
+if (!InsertStyle) {
+  llvm::errs() << llvm::toString(InsertStyle.takeError()) << "\n";
+  return 1;
+}
 auto Buffer = llvm::MemoryBuffer::getFile(FilePath);
 if (!Buffer) {
   errs() << "Couldn't open file: " + FilePath.str() + ": "
  << Buffer.getError().message() + "\n";
   return 1;
 }
 
 auto Replacements = clang::include_fixer::createIncludeFixerReplacements(
-Buffer.get()->getBuffer(), Context, InsertStyle);
+Buffer.get()->getBuffer(), Context, *InsertStyle);
 if (!Replacements) {
   errs() << "Failed to create replacement: "
  << llvm::toString(Replacements.takeError()) << "\n";
Index: clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
===
--- clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
+++ clang-tools-extra/trunk/clang-tidy/ClangTidy.cpp
@@ -197,10 +197,14 @@
   continue;
 }
 StringRef Code = Buffer.get()->getBuffer();
-format::FormatStyle Style = format::getStyle("file", File, FormatStyle);
+auto Style = format::getStyle("file", File, FormatStyle);
+if (!Style) {
+  llvm::errs() << llvm::toString(Style.takeError()) << "\n";
+  continue;
+}
 llvm::Expected CleanReplacements =
 format::cleanupAroundReplacements(Code, FileAndReplacements.second,
-  Style);
+  *Style);
 if (!CleanReplacements) {
   llvm::errs() << llvm::toString(CleanReplacements.takeError()) << "\n";
   continue;
Index: clang-tools-extra/trunk/clang-move/ClangMove.cpp
===
--- clang-tools-extra/trunk/clang-move/ClangMove.cpp
+++ clang-tools-extra/trunk/clang-move/ClangMove.cpp
@@ -742,10 +742,13 @@
 // Ignore replacements for new.h/cc.
 if (SI == FilePathToFileID.end()) continue;
 llvm::StringRef Code = SM.getBufferData(SI->second);
-format::FormatStyle Style =
-format::getStyle("file", FilePath, Context->FallbackStyle);
+auto Style = format::getStyle("file", FilePath, Context->FallbackStyle);
+if (!Style) {
+  llvm::errs() << llvm::toString(Style.takeError()) << "\n";
+  continue;
+}
 auto CleanReplacements = format::cleanupAroundReplacements(
-Code, Context->FileToReplacements[FilePath], Style);
+Code, Context->FileToReplacements[FilePath], *Style);
 
 if (!CleanReplacements) {
   llvm::errs() << llvm::toString(CleanReplacements.takeError()) << "\n";
Index: clang-tools-extra/trunk/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
===
--- clang-tools-extra/trunk/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
+++ clang-tools-extra/trunk/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
@@ -208,8 +208,15 @@
 
   // Determine a 

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL292174: clang-format: Make GetStyle return 
Expected instead of FormatStyle (authored by amaiorano).

Changed prior to commit:
  https://reviews.llvm.org/D28081?vs=84538=84610#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D28081

Files:
  cfe/trunk/include/clang/Format/Format.h
  cfe/trunk/lib/Format/Format.cpp
  cfe/trunk/lib/Tooling/Refactoring.cpp
  cfe/trunk/test/Format/style-on-command-line.cpp
  cfe/trunk/tools/clang-format/ClangFormat.cpp
  cfe/trunk/unittests/Format/FormatTest.cpp
  cfe/trunk/unittests/Format/FormatTestObjC.cpp

Index: cfe/trunk/include/clang/Format/Format.h
===
--- cfe/trunk/include/clang/Format/Format.h
+++ cfe/trunk/include/clang/Format/Format.h
@@ -853,17 +853,19 @@
 /// \param[in] FileName Path to start search for .clang-format if ``StyleName``
 /// == "file".
 /// \param[in] FallbackStyle The name of a predefined style used to fallback to
-/// in case the style can't be determined from \p StyleName.
+/// in case \p StyleName is "file" and no file can be found.
 /// \param[in] Code The actual code to be formatted. Used to determine the
 /// language if the filename isn't sufficient.
 /// \param[in] FS The underlying file system, in which the file resides. By
 /// default, the file system is the real file system.
 ///
-/// \returns FormatStyle as specified by ``StyleName``. If no style could be
-/// determined, the default is LLVM Style (see ``getLLVMStyle()``).
-FormatStyle getStyle(StringRef StyleName, StringRef FileName,
- StringRef FallbackStyle, StringRef Code = "",
- vfs::FileSystem *FS = nullptr);
+/// \returns FormatStyle as specified by ``StyleName``. If ``StyleName`` is
+/// "file" and no file is found, returns ``FallbackStyle``. If no style could be
+/// determined, returns an Error.
+llvm::Expected getStyle(StringRef StyleName, StringRef FileName,
+ StringRef FallbackStyle,
+ StringRef Code = "",
+ vfs::FileSystem *FS = nullptr);
 
 // \brief Returns a string representation of ``Language``.
 inline StringRef getLanguageName(FormatStyle::LanguageKind Language) {
Index: cfe/trunk/test/Format/style-on-command-line.cpp
===
--- cfe/trunk/test/Format/style-on-command-line.cpp
+++ cfe/trunk/test/Format/style-on-command-line.cpp
@@ -1,11 +1,11 @@
 // RUN: clang-format -style="{BasedOnStyle: Google, IndentWidth: 8}" %s | FileCheck -strict-whitespace -check-prefix=CHECK1 %s
 // RUN: clang-format -style="{BasedOnStyle: LLVM, IndentWidth: 7}" %s | FileCheck -strict-whitespace -check-prefix=CHECK2 %s
-// RUN: clang-format -style="{BasedOnStyle: invalid, IndentWidth: 7}" -fallback-style=LLVM %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK3 %s
-// RUN: clang-format -style="{lsjd}" %s -fallback-style=LLVM 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK4 %s
+// RUN: not clang-format -style="{BasedOnStyle: invalid, IndentWidth: 7}" -fallback-style=LLVM %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK3 %s
+// RUN: not clang-format -style="{lsjd}" %s -fallback-style=LLVM 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK4 %s
 // RUN: printf "BasedOnStyle: google\nIndentWidth: 5\n" > %T/.clang-format
 // RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s | FileCheck -strict-whitespace -check-prefix=CHECK5 %s
 // RUN: printf "\n" > %T/.clang-format
-// RUN: clang-format -style=file -fallback-style=webkit -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK6 %s
+// RUN: not clang-format -style=file -fallback-style=webkit -assume-filename=%T/foo.cpp < %s 2>&1 | FileCheck -strict-whitespace -check-prefix=CHECK6 %s
 // RUN: rm %T/.clang-format
 // RUN: printf "BasedOnStyle: google\nIndentWidth: 6\n" > %T/_clang-format
 // RUN: clang-format -style=file -assume-filename=%T/foo.cpp < %s | FileCheck -strict-whitespace -check-prefix=CHECK7 %s
@@ -15,13 +15,10 @@
 // CHECK1: {{^int\* i;$}}
 // CHECK2: {{^   int \*i;$}}
 // CHECK3: Unknown value for BasedOnStyle: invalid
-// CHECK3: Error parsing -style: {{I|i}}nvalid argument, using LLVM style
-// CHECK3: {{^  int \*i;$}}
-// CHECK4: Error parsing -style: {{I|i}}nvalid argument, using LLVM style
-// CHECK4: {{^  int \*i;$}}
+// CHECK3: Error parsing -style: {{I|i}}nvalid argument
+// CHECK4: Error parsing -style: {{I|i}}nvalid argument
 // CHECK5: {{^ int\* i;$}}
 // CHECK6: {{^Error reading .*\.clang-format: (I|i)nvalid argument}}
-// CHECK6: {{^int\* i;$}}
 // CHECK7: {{^  int\* i;$}}
 // CHECK8: {{^  int\* i;$}}
 // CHECK9: {{^int \*i;$}}
Index: cfe/trunk/lib/Format/Format.cpp
===
--- 

[PATCH] D28315: Update tools to use new getStyle API

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D28315#647212, @ioeric wrote:

> In https://reviews.llvm.org/D28315#647154, @amaiorano wrote:
>
> > In https://reviews.llvm.org/D28315#647103, @ioeric wrote:
> >
> > > Let me know when broken tests are fixed and this patch (and the 
> > > corresponding patch) is ready again for review. Also let me know if you 
> > > need any help.
> >
> >
> > I updated the two patches after rebasing on latest (no conflicts). These 
> > should be ready to go. If you don't mind testing the two again on Linux, 
> > that would be greatly appreciated. Otherwise, on Windows the tests pass.
>
>
> All tests passed on Linux. Are these patches still depending on 
> https://reviews.llvm.org/D28419? Or the previous issue has been somehow 
> resolved on Windows?


Thank you for testing it on Linux. No, there is no dependency on 
https://reviews.llvm.org/D28419 insofar as getting these 2 patches in. 
https://reviews.llvm.org/D28419 is about making sure tests work on Windows if 
you use Git and set core.autocrlf; however, I suspect the Windows build 
machines are using svn.


https://reviews.llvm.org/D28315



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28315: Update tools to use new getStyle API

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D28315#647103, @ioeric wrote:

> Let me know when broken tests are fixed and this patch (and the corresponding 
> patch) is ready again for review. Also let me know if you need any help.


I updated the two patches after rebasing on latest (no conflicts). These should 
be ready to go. If you don't mind testing the two again on Linux, that would be 
greatly appreciated. Otherwise, on Windows the tests pass.


https://reviews.llvm.org/D28315



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28315: Update tools to use new getStyle API

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano updated this revision to Diff 84539.
amaiorano added a comment.

Rebased changes on latest. No functional changes in this diff since last.

All tests pass (on Windows).


https://reviews.llvm.org/D28315

Files:
  change-namespace/ChangeNamespace.cpp
  clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
  clang-move/ClangMove.cpp
  clang-tidy/ClangTidy.cpp
  include-fixer/tool/ClangIncludeFixer.cpp

Index: include-fixer/tool/ClangIncludeFixer.cpp
===
--- include-fixer/tool/ClangIncludeFixer.cpp
+++ include-fixer/tool/ClangIncludeFixer.cpp
@@ -303,10 +303,13 @@
const IncludeFixerContext::HeaderInfo ) {
   return LHS.QualifiedName == RHS.QualifiedName;
 });
-format::FormatStyle InsertStyle =
-format::getStyle("file", Context.getFilePath(), Style);
+auto InsertStyle = format::getStyle("file", Context.getFilePath(), Style);
+if (!InsertStyle) {
+  llvm::errs() << llvm::toString(InsertStyle.takeError()) << "\n";
+  return 1;
+}
 auto Replacements = clang::include_fixer::createIncludeFixerReplacements(
-Code->getBuffer(), Context, InsertStyle,
+Code->getBuffer(), Context, *InsertStyle,
 /*AddQualifiers=*/IsUniqueQualifiedName);
 if (!Replacements) {
   errs() << "Failed to create replacements: "
@@ -378,16 +381,20 @@
   std::vector FixerReplacements;
   for (const auto  : Contexts) {
 StringRef FilePath = Context.getFilePath();
-format::FormatStyle InsertStyle = format::getStyle("file", FilePath, Style);
+auto InsertStyle = format::getStyle("file", FilePath, Style);
+if (!InsertStyle) {
+  llvm::errs() << llvm::toString(InsertStyle.takeError()) << "\n";
+  return 1;
+}
 auto Buffer = llvm::MemoryBuffer::getFile(FilePath);
 if (!Buffer) {
   errs() << "Couldn't open file: " + FilePath.str() + ": "
  << Buffer.getError().message() + "\n";
   return 1;
 }
 
 auto Replacements = clang::include_fixer::createIncludeFixerReplacements(
-Buffer.get()->getBuffer(), Context, InsertStyle);
+Buffer.get()->getBuffer(), Context, *InsertStyle);
 if (!Replacements) {
   errs() << "Failed to create replacement: "
  << llvm::toString(Replacements.takeError()) << "\n";
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -197,10 +197,14 @@
   continue;
 }
 StringRef Code = Buffer.get()->getBuffer();
-format::FormatStyle Style = format::getStyle("file", File, FormatStyle);
+auto Style = format::getStyle("file", File, FormatStyle);
+if (!Style) {
+  llvm::errs() << llvm::toString(Style.takeError()) << "\n";
+  continue;
+}
 llvm::Expected CleanReplacements =
 format::cleanupAroundReplacements(Code, FileAndReplacements.second,
-  Style);
+  *Style);
 if (!CleanReplacements) {
   llvm::errs() << llvm::toString(CleanReplacements.takeError()) << "\n";
   continue;
Index: clang-move/ClangMove.cpp
===
--- clang-move/ClangMove.cpp
+++ clang-move/ClangMove.cpp
@@ -742,10 +742,13 @@
 // Ignore replacements for new.h/cc.
 if (SI == FilePathToFileID.end()) continue;
 llvm::StringRef Code = SM.getBufferData(SI->second);
-format::FormatStyle Style =
-format::getStyle("file", FilePath, Context->FallbackStyle);
+auto Style = format::getStyle("file", FilePath, Context->FallbackStyle);
+if (!Style) {
+  llvm::errs() << llvm::toString(Style.takeError()) << "\n";
+  continue;
+}
 auto CleanReplacements = format::cleanupAroundReplacements(
-Code, Context->FileToReplacements[FilePath], Style);
+Code, Context->FileToReplacements[FilePath], *Style);
 
 if (!CleanReplacements) {
   llvm::errs() << llvm::toString(CleanReplacements.takeError()) << "\n";
Index: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
===
--- clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
+++ clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
@@ -208,8 +208,15 @@
 
   // Determine a formatting style from options.
   format::FormatStyle FormatStyle;
-  if (DoFormat)
-FormatStyle = format::getStyle(FormatStyleOpt, FormatStyleConfig, "LLVM");
+  if (DoFormat) {
+auto FormatStyleOrError =
+format::getStyle(FormatStyleOpt, FormatStyleConfig, "LLVM");
+if (!FormatStyleOrError) {
+  llvm::errs() << llvm::toString(FormatStyleOrError.takeError()) << "\n";
+  return 1;
+}
+FormatStyle = *FormatStyleOrError;
+  }
 
   TUReplacements TURs;
   

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano updated this revision to Diff 84538.
amaiorano added a comment.

Rebased changes on latest, no functional change in this diff.


https://reviews.llvm.org/D28081

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Tooling/Refactoring.cpp
  test/Format/style-on-command-line.cpp
  tools/clang-format/ClangFormat.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestObjC.cpp

Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -68,17 +68,21 @@
   FormatStyle Style;
 };
 
-TEST_F(FormatTestObjC, DetectsObjCInHeaders) {
-  Style = getStyle("LLVM", "a.h", "none", "@interface\n"
-  "- (id)init;");
-  EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
+TEST(FormatTestObjCStyle, DetectsObjCInHeaders) {
+  auto Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+   "- (id)init;");
+  ASSERT_TRUE((bool)Style);
+  EXPECT_EQ(FormatStyle::LK_ObjC, Style->Language);
+
   Style = getStyle("LLVM", "a.h", "none", "@interface\n"
   "+ (id)init;");
-  EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
+  ASSERT_TRUE((bool)Style);
+  EXPECT_EQ(FormatStyle::LK_ObjC, Style->Language);
 
   // No recognizable ObjC.
   Style = getStyle("LLVM", "a.h", "none", "void f() {}");
-  EXPECT_EQ(FormatStyle::LK_Cpp, Style.Language);
+  ASSERT_TRUE((bool)Style);
+  EXPECT_EQ(FormatStyle::LK_Cpp, Style->Language);
 }
 
 TEST_F(FormatTestObjC, FormatObjCTryCatch) {
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10967,22 +10967,51 @@
   ASSERT_TRUE(
   FS.addFile("/a/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style1 = getStyle("file", "/a/.clang-format", "Google", "", );
-  ASSERT_EQ(Style1, getLLVMStyle());
+  ASSERT_TRUE((bool)Style1);
+  ASSERT_EQ(*Style1, getLLVMStyle());
 
   // Test 2: fallback to default.
   ASSERT_TRUE(
   FS.addFile("/b/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style2 = getStyle("file", "/b/test.cpp", "Mozilla", "", );
-  ASSERT_EQ(Style2, getMozillaStyle());
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getMozillaStyle());
 
   // Test 3: format file in parent directory.
   ASSERT_TRUE(
   FS.addFile("/c/.clang-format", 0,
  llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
   ASSERT_TRUE(FS.addFile("/c/sub/sub/sub/test.cpp", 0,
  llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style3 = getStyle("file", "/c/sub/sub/sub/test.cpp", "LLVM", "", );
-  ASSERT_EQ(Style3, getGoogleStyle());
+  ASSERT_TRUE((bool)Style3);
+  ASSERT_EQ(*Style3, getGoogleStyle());
+
+  // Test 4: error on invalid fallback style
+  auto Style4 = getStyle("file", "a.h", "KungFu", "", );
+  ASSERT_FALSE((bool)Style4);
+  llvm::consumeError(Style4.takeError());
+
+  // Test 5: error on invalid yaml on command line
+  auto Style5 = getStyle("{invalid_key=invalid_value}", "a.h", "LLVM", "", );
+  ASSERT_FALSE((bool)Style5);
+  llvm::consumeError(Style5.takeError());
+
+  // Test 6: error on invalid style
+  auto Style6 = getStyle("KungFu", "a.h", "LLVM", "", );
+  ASSERT_FALSE((bool)Style6);
+  llvm::consumeError(Style6.takeError());
+
+  // Test 7: found config file, error on parsing it
+  ASSERT_TRUE(
+  FS.addFile("/d/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM\n"
+  "InvalidKey: InvalidValue")));
+  ASSERT_TRUE(
+  FS.addFile("/d/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style7 = getStyle("file", "/d/.clang-format", "LLVM", "", );
+  ASSERT_FALSE((bool)Style7);
+  llvm::consumeError(Style7.takeError());
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: tools/clang-format/ClangFormat.cpp
===
--- tools/clang-format/ClangFormat.cpp
+++ tools/clang-format/ClangFormat.cpp
@@ -249,12 +249,17 @@
   if (fillRanges(Code.get(), Ranges))
 return true;
   StringRef AssumedFileName = (FileName == "-") ? AssumeFileName : FileName;
-  FormatStyle FormatStyle =
+
+  llvm::Expected FormatStyle =
   getStyle(Style, AssumedFileName, FallbackStyle, Code->getBuffer());
+  if (!FormatStyle) {
+llvm::errs() << llvm::toString(FormatStyle.takeError()) << "\n";
+return true;
+  }
   if (SortIncludes.getNumOccurrences() != 0)
-FormatStyle.SortIncludes = SortIncludes;
+FormatStyle->SortIncludes = SortIncludes;
   unsigned CursorPosition = Cursor;
-  Replacements Replaces = sortIncludes(FormatStyle, Code->getBuffer(), Ranges,
+  Replacements Replaces = sortIncludes(*FormatStyle, 

[PATCH] D28315: Update tools to use new getStyle API

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added inline comments.



Comment at: change-namespace/ChangeNamespace.cpp:892
+  llvm::errs() << llvm::toString(Style.takeError()) << "\n";
+  continue;
+}

amaiorano wrote:
> amaiorano wrote:
> > ioeric wrote:
> > > I'd still like to apply replacements that are not cleaned up. Could you 
> > > add the following line before continue, thanks! :)
> > > ```
> > > FileToReplacements[FilePath] = Replaces;
> > > ```
> > Will do.
> Ah darn, just realized that I forgot to make this change you asked for 
> @ioeric. Let me know if everything else is okay, and I'll be sure to add this 
> in before submitting. 
@ioeric Was looking at this code, and I'm wondering about the change you wanted 
me to make here. You said that you'd still like to apply replacements that are 
not cleaned up, so shouldn't that also be the case for when 
format::cleanupAroundReplacements fails (just below)? In other words, shouldn't 
it be:

```
// Clean up old namespaces if there is nothing in it after moving.
auto CleanReplacements =
format::cleanupAroundReplacements(Code, Replaces, *Style);
if (!CleanReplacements) {
  llvm::errs() << llvm::toString(CleanReplacements.takeError()) << "\n";
  FileToReplacements[FilePath] = Replaces;
  continue;
}
FileToReplacements[FilePath] = *CleanReplacements;
  }
```

If so, I propose instead that we add 
```
FileToReplacements[FilePath] = Replaces;
``` 
before the call to format::getStyle so that if either of the format-related 
calls below it (getStyle or cleanupAroundReplacements) fail, we get the unclean 
replacements.


https://reviews.llvm.org/D28315



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28315: Update tools to use new getStyle API

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added inline comments.



Comment at: change-namespace/ChangeNamespace.cpp:892
+  llvm::errs() << llvm::toString(Style.takeError()) << "\n";
+  continue;
+}

amaiorano wrote:
> ioeric wrote:
> > I'd still like to apply replacements that are not cleaned up. Could you add 
> > the following line before continue, thanks! :)
> > ```
> > FileToReplacements[FilePath] = Replaces;
> > ```
> Will do.
Ah darn, just realized that I forgot to make this change you asked for @ioeric. 
Let me know if everything else is okay, and I'll be sure to add this in before 
submitting. 


https://reviews.llvm.org/D28315



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28315: Update tools to use new getStyle API

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano updated this revision to Diff 83952.
amaiorano added a comment.

Fixed code in ClangApplyReplacementsMain.cpp so that we only handle the 

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano updated this revision to Diff 83949.
amaiorano added a comment.

Fixed Format/style-on-command-line.cpp test to match new expected output.


https://reviews.llvm.org/D28081

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Tooling/Refactoring.cpp
  test/Format/style-on-command-line.cpp
  tools/clang-format/ClangFormat.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestObjC.cpp

Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -68,17 +68,21 @@
   FormatStyle Style;
 };
 
-TEST_F(FormatTestObjC, DetectsObjCInHeaders) {
-  Style = getStyle("LLVM", "a.h", "none", "@interface\n"
-  "- (id)init;");
-  EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
+TEST(FormatTestObjCStyle, DetectsObjCInHeaders) {
+  auto Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+   "- (id)init;");
+  ASSERT_TRUE((bool)Style);
+  EXPECT_EQ(FormatStyle::LK_ObjC, Style->Language);
+
   Style = getStyle("LLVM", "a.h", "none", "@interface\n"
   "+ (id)init;");
-  EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
+  ASSERT_TRUE((bool)Style);
+  EXPECT_EQ(FormatStyle::LK_ObjC, Style->Language);
 
   // No recognizable ObjC.
   Style = getStyle("LLVM", "a.h", "none", "void f() {}");
-  EXPECT_EQ(FormatStyle::LK_Cpp, Style.Language);
+  ASSERT_TRUE((bool)Style);
+  EXPECT_EQ(FormatStyle::LK_Cpp, Style->Language);
 }
 
 TEST_F(FormatTestObjC, FormatObjCTryCatch) {
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10949,22 +10949,51 @@
   ASSERT_TRUE(
   FS.addFile("/a/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style1 = getStyle("file", "/a/.clang-format", "Google", "", );
-  ASSERT_EQ(Style1, getLLVMStyle());
+  ASSERT_TRUE((bool)Style1);
+  ASSERT_EQ(*Style1, getLLVMStyle());
 
   // Test 2: fallback to default.
   ASSERT_TRUE(
   FS.addFile("/b/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style2 = getStyle("file", "/b/test.cpp", "Mozilla", "", );
-  ASSERT_EQ(Style2, getMozillaStyle());
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getMozillaStyle());
 
   // Test 3: format file in parent directory.
   ASSERT_TRUE(
   FS.addFile("/c/.clang-format", 0,
  llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
   ASSERT_TRUE(FS.addFile("/c/sub/sub/sub/test.cpp", 0,
  llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style3 = getStyle("file", "/c/sub/sub/sub/test.cpp", "LLVM", "", );
-  ASSERT_EQ(Style3, getGoogleStyle());
+  ASSERT_TRUE((bool)Style3);
+  ASSERT_EQ(*Style3, getGoogleStyle());
+
+  // Test 4: error on invalid fallback style
+  auto Style4 = getStyle("file", "a.h", "KungFu", "", );
+  ASSERT_FALSE((bool)Style4);
+  llvm::consumeError(Style4.takeError());
+
+  // Test 5: error on invalid yaml on command line
+  auto Style5 = getStyle("{invalid_key=invalid_value}", "a.h", "LLVM", "", );
+  ASSERT_FALSE((bool)Style5);
+  llvm::consumeError(Style5.takeError());
+
+  // Test 6: error on invalid style
+  auto Style6 = getStyle("KungFu", "a.h", "LLVM", "", );
+  ASSERT_FALSE((bool)Style6);
+  llvm::consumeError(Style6.takeError());
+
+  // Test 7: found config file, error on parsing it
+  ASSERT_TRUE(
+  FS.addFile("/d/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM\n"
+  "InvalidKey: InvalidValue")));
+  ASSERT_TRUE(
+  FS.addFile("/d/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style7 = getStyle("file", "/d/.clang-format", "LLVM", "", );
+  ASSERT_FALSE((bool)Style7);
+  llvm::consumeError(Style7.takeError());
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: tools/clang-format/ClangFormat.cpp
===
--- tools/clang-format/ClangFormat.cpp
+++ tools/clang-format/ClangFormat.cpp
@@ -249,12 +249,17 @@
   if (fillRanges(Code.get(), Ranges))
 return true;
   StringRef AssumedFileName = (FileName == "-") ? AssumeFileName : FileName;
-  FormatStyle FormatStyle =
+
+  llvm::Expected FormatStyle =
   getStyle(Style, AssumedFileName, FallbackStyle, Code->getBuffer());
+  if (!FormatStyle) {
+llvm::errs() << llvm::toString(FormatStyle.takeError()) << "\n";
+return true;
+  }
   if (SortIncludes.getNumOccurrences() != 0)
-FormatStyle.SortIncludes = SortIncludes;
+FormatStyle->SortIncludes = SortIncludes;
   unsigned CursorPosition = Cursor;
-  Replacements Replaces = sortIncludes(FormatStyle, Code->getBuffer(), Ranges,
+  Replacements Replaces = 

[PATCH] D28419: clang-tools-extra: add gitattributes to disable EOL conversions on checkout of test source files

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

Last night, I realized that this problem extends to the clang/tests as well, 
not just those in clang-tools-extra. Is there someone who knows Windows, Git, 
and the testing pipeline that can weigh in more on this topic?


https://reviews.llvm.org/D28419



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28315: Update tools to use new getStyle API

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D28315#641032, @amaiorano wrote:

> In https://reviews.llvm.org/D28315#639559, @ioeric wrote:
>
> > I ran `ninja check-all` with https://reviews.llvm.org/D28081 and 
> > https://reviews.llvm.org/D28315 (in Linux), and some lit tests failed, 
> > namely:
> >
> >   Clang :: Format/style-on-command-line.cpp
>
>
> This was the test that is explicitly disabled on Windows. I enabled it 
> locally and modified the test to match the modified output and expected 
> non-zero result (patch forth-coming once I understand what's up with the 
> failed tests you mention below...)
>
> >   Clang Tools :: clang-apply-replacements/basic.cpp
> >   Clang Tools :: clang-apply-replacements/conflict.cpp
> >   Clang Tools :: clang-apply-replacements/crlf.cpp
> >   Clang Tools :: clang-apply-replacements/format.cpp
> >   Clang Tools :: clang-rename/ClassReplacements.cpp
> > 
> >   Error message I am seeing: `Expected must be checked before access or 
> > destruction.`
> >   
> >   Could you double check? Thanks!
>
> These tests pass for me. For example, when I run llvm-lit explicitly on 
> clang-apply-replacements/basic.cpp, I get:
>
>   C:\code\llvm-build-msvc\Debug\bin>llvm-lit.py -a -v 
> c:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements\basic.cpp
>   -- Testing: 1 tests, 1 threads --
>   PASS: Clang Tools :: clang-apply-replacements/basic.cpp (1 of 1)
>   Script:
>   --
>   mkdir -p 
> C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
>   grep -Ev "// *[A-Z-]+:" 
> C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h
>  > 
> C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h
>   sed 
> "s#\$(path)#C:/code/llvm-build-msvc/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#"
>  
> C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/file1.yaml
>  > 
> C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/file1.yaml
>   sed 
> "s#\$(path)#C:/code/llvm-build-msvc/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#"
>  
> C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/file2.yaml
>  > 
> C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/file2.yaml
>   clang-apply-replacements 
> C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
>   FileCheck 
> -input-file=C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h
>  
> C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h
>   ls -1 
> C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
>  | FileCheck 
> C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements\basic.cpp 
> --check-prefix=YAML
>   grep -Ev "// *[A-Z-]+:" 
> C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h
>  > 
> C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h
>   clang-apply-replacements -remove-change-desc-files 
> C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
>   ls -1 
> C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
>  | FileCheck 
> C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements\basic.cpp 
> --check-prefix=NO_YAML
>   --
>   Exit Code: 0
>  
>   Command Output (stdout):
>   --
>   $ "mkdir" "-p" 
> "C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
>   $ "grep" "-Ev" "// *[A-Z-]+:" 
> "C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h"
>   $ "sed" 
> "s#\$(path)#C:/code/llvm-build-msvc/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#"
>  
> "C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/file1.yaml"
>   $ "sed" 
> "s#\$(path)#C:/code/llvm-build-msvc/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#"
>  
> "C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/file2.yaml"
>   $ "clang-apply-replacements" 
> "C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
>   $ "FileCheck" 
> "-input-file=C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h"
>  
> "C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h"
>   $ "ls" "-1" 
> "C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
>   $ "FileCheck" 
> 

[PATCH] D28315: Update tools to use new getStyle API

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D28315#639559, @ioeric wrote:

> I ran `ninja check-all` with https://reviews.llvm.org/D28081 and 
> https://reviews.llvm.org/D28315 (in Linux), and some lit tests failed, namely:
>
>   Clang :: Format/style-on-command-line.cpp


This was the test that is explicitly disabled on Windows. I enabled it locally 
and modified the test to match the modified output and expected non-zero result 
(patch forth-coming once I understand what's up with the failed tests you 
mention below...)

>   Clang Tools :: clang-apply-replacements/basic.cpp
>   Clang Tools :: clang-apply-replacements/conflict.cpp
>   Clang Tools :: clang-apply-replacements/crlf.cpp
>   Clang Tools :: clang-apply-replacements/format.cpp
>   Clang Tools :: clang-rename/ClassReplacements.cpp
> 
>   Error message I am seeing: `Expected must be checked before access or 
> destruction.`
>   
>   Could you double check? Thanks!

These tests pass for me. For example, when I run llvm-lit explicitly on 
clang-apply-replacements/basic.cpp, I get:

  C:\code\llvm-build-msvc\Debug\bin>llvm-lit.py -a -v 
c:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements\basic.cpp
  -- Testing: 1 tests, 1 threads --
  PASS: Clang Tools :: clang-apply-replacements/basic.cpp (1 of 1)
  Script:
  --
  mkdir -p 
C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
  grep -Ev "// *[A-Z-]+:" 
C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h
 > 
C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h
  sed 
"s#\$(path)#C:/code/llvm-build-msvc/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#"
 
C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/file1.yaml
 > 
C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/file1.yaml
  sed 
"s#\$(path)#C:/code/llvm-build-msvc/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#"
 
C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/file2.yaml
 > 
C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/file2.yaml
  clang-apply-replacements 
C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
  FileCheck 
-input-file=C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h
 
C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h
  ls -1 
C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
 | FileCheck 
C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements\basic.cpp 
--check-prefix=YAML
  grep -Ev "// *[A-Z-]+:" 
C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h
 > 
C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h
  clang-apply-replacements -remove-change-desc-files 
C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
  ls -1 
C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic
 | FileCheck 
C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements\basic.cpp 
--check-prefix=NO_YAML
  --
  Exit Code: 0
  
  Command Output (stdout):
  --
  $ "mkdir" "-p" 
"C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
  $ "grep" "-Ev" "// *[A-Z-]+:" 
"C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h"
  $ "sed" 
"s#\$(path)#C:/code/llvm-build-msvc/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#"
 
"C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/file1.yaml"
  $ "sed" 
"s#\$(path)#C:/code/llvm-build-msvc/tools/clang/tools/extra/test/clang-apply-replacements/Output/Inputs/basic#"
 
"C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/file2.yaml"
  $ "clang-apply-replacements" 
"C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
  $ "FileCheck" 
"-input-file=C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic/basic.h"
 
"C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h"
  $ "ls" "-1" 
"C:\code\llvm-build-msvc\tools\clang\tools\extra\test\clang-apply-replacements\Output/Inputs/basic"
  $ "FileCheck" 
"C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements\basic.cpp" 
"--check-prefix=YAML"
  $ "grep" "-Ev" "// *[A-Z-]+:" 
"C:\code\llvm\tools\clang\tools\extra\test\clang-apply-replacements/Inputs/basic/basic.h"
  $ "clang-apply-replacements" "-remove-change-desc-files" 

[PATCH] D28315: Update tools to use new getStyle API

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D28315#639559, @ioeric wrote:

> I ran `ninja check-all` with https://reviews.llvm.org/D28081 and 
> https://reviews.llvm.org/D28315 (in Linux), and some lit tests failed, namely:
>
>   Clang :: Format/style-on-command-line.cpp
>   Clang Tools :: clang-apply-replacements/basic.cpp
>   Clang Tools :: clang-apply-replacements/conflict.cpp
>   Clang Tools :: clang-apply-replacements/crlf.cpp
>   Clang Tools :: clang-apply-replacements/format.cpp
>   Clang Tools :: clang-rename/ClassReplacements.cpp
>   
>
> Error message I am seeing: `Expected must be checked before access or 
> destruction.`
>
> Could you double check? Thanks!


Will definitely double check. Strange that I didn't get these errors on 
Windows. I did notice in the output that it said one test was disabled. I'll 
take a closer look asap.




Comment at: change-namespace/ChangeNamespace.cpp:892
+  llvm::errs() << llvm::toString(Style.takeError()) << "\n";
+  continue;
+}

ioeric wrote:
> I'd still like to apply replacements that are not cleaned up. Could you add 
> the following line before continue, thanks! :)
> ```
> FileToReplacements[FilePath] = Replaces;
> ```
Will do.


https://reviews.llvm.org/D28315



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28419: clang-tools-extra: add gitattributes to disable EOL conversions on checkout of test source files

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

Hello, sorry for the lack of details here. I will re-run the tests later and 
report here the tests that are failing along with output.

The main issue is that when using Git on Windows, by default the installer will 
set core.autocrlf to true, which is a Git config variable that tells Git to 
automatically convert line endings to CRLF for text files on checkout, and back 
to LF on staging. Some of the tests expect LF line endings - for instance, when 
specifying absolute offsets for replacements, these offsets are incorrect when 
there are extra "CR" bytes in the file, which end up making the tests fail 
since the output doesn't match the expected output.

Note that on Linux, I believe core.autocrlf isn't usually set, so it defaults 
to false (could someone verify this for me?). Since files in Git are usually 
'LF', this works fine for Linux users. On Windows, you usually want the CRLF 
line endings on source files because not all Windows text editors are good at 
being consistent with line endings.

The solution I propose here is to add a single .gitattributes file that 
specifies that all .h and .cpp files under clang/tools/extra/test should be 
checked out with whatever line endings they were checked in with. In other 
words, don't convert line endings at all for these files. This means existing 
test source files that have LF will continue to have LF when checked out in 
Git, or if they have CRLF (which is the case for crlf.cpp), it will be checked 
out with CRLF line endings.

Another consequence of this change is that if a Windows developer decides to 
add a new test, they'd create a test source file with CRLF line endings, and 
this file would be committed with these CRLF line endings. This would work fine 
on Linux since the file would be checked out as-is as well (with CRLF line 
endings), and the test would run fine. However, without this change, the 
Windows developer would write a test that works fine on Windows, then when 
staging the file, the CRLF would be converted to LF and the test would fail on 
Linux.

Alternative solutions:

1. Instead of applying the no eol conversion to ALL .h/.cpp under test, we 
could add explicit entries to the .h/.cpp files that specifically require them 
for their tests (those that specify offsets for replacements, for e.g.)
2. Instead of a single .gitattributes file at the root of the extra/ directory, 
we can add one to each test directory that requires it, specifying the exact 
files in that test directory that needs to not apply eol conversions.

Again, I will post the specific failures as soon as I get the chance.


https://reviews.llvm.org/D28419



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28419: clang-tools-extra: add gitattributes to disable EOL conversions on checkout of test source files

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano created this revision.
amaiorano added reviewers: malcolm.parsons, ioeric.
amaiorano added a subscriber: cfe-commits.

Certain tests expect their input files to have LF line endings, while others 
expect CRLF. On Windows, when using Git with core.autocrlf=true, these tests 
fail because all source files are checked out with CRLF line endings. This 
change makes sure that these files are not converted at all on checkout.


https://reviews.llvm.org/D28419

Files:
  .gitattributes


Index: .gitattributes
===
--- /dev/null
+++ .gitattributes
@@ -0,0 +1,5 @@
+text=auto
+
+# Disable eol conversions as certain tests rely on specific line endings
+test/**/*.h -text
+test/**/*.cpp -text


Index: .gitattributes
===
--- /dev/null
+++ .gitattributes
@@ -0,0 +1,5 @@
+text=auto
+
+# Disable eol conversions as certain tests rely on specific line endings
+test/**/*.h -text
+test/**/*.cpp -text
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28315: Update tools to use new getStyle API

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

@ioeric Okay, check-clang-tools passes with my changes.

FYI, tests were originally failing for me (without my changes) because I use 
Git, and it's configured to convert line endings to Windows (crlf) on checkout, 
but some of the tools tests specify extract offsets into source files, which 
end up being incorrect if you've got an extra byte per line ;) I'll open a 
ticket/discussion about it separately.

So if you give me the go ahead, I'll submit this change _after_ submitting 
https://reviews.llvm.org/D28081.


https://reviews.llvm.org/D28315



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28315: Update tools to use new getStyle API

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D28315#636888, @hokein wrote:

> In https://reviews.llvm.org/D28315#636821, @amaiorano wrote:
>
> > Is there a way to run tests without ninja? I'm on Windows. If not, I'll use 
> > my Linux VM.
>
>
> It is not related to the build system. There should be a `check-clang-tools` 
> project, you can run tests by building it.  For example, if you are using 
> make, just run `make check-clang-tools`; If you are using Visual Studio, 
> build the `check-clang-tools` project.


Yes, thanks for that. Unfortunately, the tests don't all pass when run using 
Visual Studio. I installed the requisite GNU utils, but get 25 failures. Will 
investigate a bit more, otherwise will use Linux.


https://reviews.llvm.org/D28315



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28315: Update tools to use new getStyle API

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D28315#636670, @hokein wrote:

> In https://reviews.llvm.org/D28315#635865, @amaiorano wrote:
>
> > I made these changes, and they build, but I didn't really test them. Are 
> > there unit tests for these tools that I can run?
>
>
> If you use ninja build, you can run `ninja check-clang-tools` to run all 
> tests.


Is there a way to run tests without ninja? I'm on Windows. If not, I'll use my 
Linux VM.


https://reviews.llvm.org/D28315



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

@ioeric I think you removed cfe-commits as a reviewer, then added it back, but 
it didn't work. Should I re-add?


https://reviews.llvm.org/D28081



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28315: Update tools to use new getStyle API

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

I made these changes, and they build, but I didn't really test them. Are there 
unit tests for these tools that I can run?


https://reviews.llvm.org/D28315



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28315: Update tools to use new getStyle API

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano created this revision.
amaiorano added reviewers: ioeric, cfe-commits, klimek, djasper.

See https://reviews.llvm.org/D28081 for the changes to getStyle

This change will be committed right after https://reviews.llvm.org/D28081.


https://reviews.llvm.org/D28315

Files:
  change-namespace/ChangeNamespace.cpp
  clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
  clang-move/ClangMove.cpp
  clang-tidy/ClangTidy.cpp
  include-fixer/tool/ClangIncludeFixer.cpp

Index: include-fixer/tool/ClangIncludeFixer.cpp
===
--- include-fixer/tool/ClangIncludeFixer.cpp
+++ include-fixer/tool/ClangIncludeFixer.cpp
@@ -297,10 +297,13 @@
const IncludeFixerContext::HeaderInfo ) {
   return LHS.QualifiedName == RHS.QualifiedName;
 });
-format::FormatStyle InsertStyle =
-format::getStyle("file", Context.getFilePath(), Style);
+auto InsertStyle = format::getStyle("file", Context.getFilePath(), Style);
+if (!InsertStyle) {
+  llvm::errs() << llvm::toString(InsertStyle.takeError()) << "\n";
+  return 1;
+}
 auto Replacements = clang::include_fixer::createIncludeFixerReplacements(
-Code->getBuffer(), Context, InsertStyle,
+Code->getBuffer(), Context, *InsertStyle,
 /*AddQualifiers=*/IsUniqueQualifiedName);
 if (!Replacements) {
   errs() << "Failed to create replacements: "
@@ -371,16 +374,20 @@
   std::vector FixerReplacements;
   for (const auto  : Contexts) {
 StringRef FilePath = Context.getFilePath();
-format::FormatStyle InsertStyle = format::getStyle("file", FilePath, Style);
+auto InsertStyle = format::getStyle("file", FilePath, Style);
+if (!InsertStyle) {
+  llvm::errs() << llvm::toString(InsertStyle.takeError()) << "\n";
+  return 1;
+}
 auto Buffer = llvm::MemoryBuffer::getFile(FilePath);
 if (!Buffer) {
   errs() << "Couldn't open file: " + FilePath.str() + ": "
  << Buffer.getError().message() + "\n";
   return 1;
 }
 
 auto Replacements = clang::include_fixer::createIncludeFixerReplacements(
-Buffer.get()->getBuffer(), Context, InsertStyle);
+Buffer.get()->getBuffer(), Context, *InsertStyle);
 if (!Replacements) {
   errs() << "Failed to create replacement: "
  << llvm::toString(Replacements.takeError()) << "\n";
Index: clang-tidy/ClangTidy.cpp
===
--- clang-tidy/ClangTidy.cpp
+++ clang-tidy/ClangTidy.cpp
@@ -197,10 +197,14 @@
   continue;
 }
 StringRef Code = Buffer.get()->getBuffer();
-format::FormatStyle Style = format::getStyle("file", File, FormatStyle);
+auto Style = format::getStyle("file", File, FormatStyle);
+if (!Style) {
+  llvm::errs() << llvm::toString(Style.takeError()) << "\n";
+  continue;
+}
 llvm::Expected CleanReplacements =
 format::cleanupAroundReplacements(Code, FileAndReplacements.second,
-  Style);
+  *Style);
 if (!CleanReplacements) {
   llvm::errs() << llvm::toString(CleanReplacements.takeError()) << "\n";
   continue;
Index: clang-move/ClangMove.cpp
===
--- clang-move/ClangMove.cpp
+++ clang-move/ClangMove.cpp
@@ -712,10 +712,13 @@
 // Ignore replacements for new.h/cc.
 if (SI == FilePathToFileID.end()) continue;
 llvm::StringRef Code = SM.getBufferData(SI->second);
-format::FormatStyle Style =
-format::getStyle("file", FilePath, Context->FallbackStyle);
+auto Style = format::getStyle("file", FilePath, Context->FallbackStyle);
+if (!Style) {
+  llvm::errs() << llvm::toString(Style.takeError()) << "\n";
+  continue;
+}
 auto CleanReplacements = format::cleanupAroundReplacements(
-Code, Context->FileToReplacements[FilePath], Style);
+Code, Context->FileToReplacements[FilePath], *Style);
 
 if (!CleanReplacements) {
   llvm::errs() << llvm::toString(CleanReplacements.takeError()) << "\n";
Index: clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
===
--- clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
+++ clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
@@ -207,9 +207,14 @@
   IntrusiveRefCntPtr(new DiagnosticIDs()), DiagOpts.get());
 
   // Determine a formatting style from options.
-  format::FormatStyle FormatStyle;
-  if (DoFormat)
+  llvm::Expected FormatStyle = format::getNoStyle();
+  if (DoFormat) {
 FormatStyle = format::getStyle(FormatStyleOpt, FormatStyleConfig, "LLVM");
+if (!FormatStyle) {
+  llvm::errs() << llvm::toString(FormatStyle.takeError()) << "\n";
+  return 1;
+}
+  }
 
   

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D28081#633786, @ioeric wrote:

> The patch LGTM now. I'll accept both this and the one for clang-tool-extra 
> when it is ready.
>
> Regarding builbots, we have bots that continually run builds/tests 
> (http://lab.llvm.org:8011/). Many buildbots test llvm and clang as well as 
> clang-tools-extra (e.g. with `ninja check-all`) at some revision. Also note 
> that although llvm/clang and clang-tools-extra are different repos, they do 
> share the same revision sequence. So if clang-tools-extra is in a 
> inconsistent state, many buildbots can fail and affect llvm/clang builds. 
> Unfortunately, there is no atomic way to commit two revisions to two 
> repositories, so we just commit them quickly one after another so that we do 
> less damage. Do you have commit access to LLVM btw?


I do have commit access. I'll get to work on the clang-tools-extras and open a 
new review for it once it's ready. Thanks.


https://reviews.llvm.org/D28081



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano updated this revision to Diff 82896.
amaiorano added a comment.

Minor comment change, turned the ObjC test into a non-fixture test, and renamed 
FormatStyleOrError to FormatStyle in format function.


https://reviews.llvm.org/D28081

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Tooling/Refactoring.cpp
  tools/clang-format/ClangFormat.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestObjC.cpp

Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -68,17 +68,21 @@
   FormatStyle Style;
 };
 
-TEST_F(FormatTestObjC, DetectsObjCInHeaders) {
-  Style = getStyle("LLVM", "a.h", "none", "@interface\n"
-  "- (id)init;");
-  EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
+TEST(FormatTestObjCStyle, DetectsObjCInHeaders) {
+  auto Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+   "- (id)init;");
+  ASSERT_TRUE((bool)Style);
+  EXPECT_EQ(FormatStyle::LK_ObjC, Style->Language);
+
   Style = getStyle("LLVM", "a.h", "none", "@interface\n"
   "+ (id)init;");
-  EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
+  ASSERT_TRUE((bool)Style);
+  EXPECT_EQ(FormatStyle::LK_ObjC, Style->Language);
 
   // No recognizable ObjC.
   Style = getStyle("LLVM", "a.h", "none", "void f() {}");
-  EXPECT_EQ(FormatStyle::LK_Cpp, Style.Language);
+  ASSERT_TRUE((bool)Style);
+  EXPECT_EQ(FormatStyle::LK_Cpp, Style->Language);
 }
 
 TEST_F(FormatTestObjC, FormatObjCTryCatch) {
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10945,22 +10945,51 @@
   ASSERT_TRUE(
   FS.addFile("/a/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style1 = getStyle("file", "/a/.clang-format", "Google", "", );
-  ASSERT_EQ(Style1, getLLVMStyle());
+  ASSERT_TRUE((bool)Style1);
+  ASSERT_EQ(*Style1, getLLVMStyle());
 
   // Test 2: fallback to default.
   ASSERT_TRUE(
   FS.addFile("/b/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style2 = getStyle("file", "/b/test.cpp", "Mozilla", "", );
-  ASSERT_EQ(Style2, getMozillaStyle());
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getMozillaStyle());
 
   // Test 3: format file in parent directory.
   ASSERT_TRUE(
   FS.addFile("/c/.clang-format", 0,
  llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
   ASSERT_TRUE(FS.addFile("/c/sub/sub/sub/test.cpp", 0,
  llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style3 = getStyle("file", "/c/sub/sub/sub/test.cpp", "LLVM", "", );
-  ASSERT_EQ(Style3, getGoogleStyle());
+  ASSERT_TRUE((bool)Style3);
+  ASSERT_EQ(*Style3, getGoogleStyle());
+
+  // Test 4: error on invalid fallback style
+  auto Style4 = getStyle("file", "a.h", "KungFu", "", );
+  ASSERT_FALSE((bool)Style4);
+  llvm::consumeError(Style4.takeError());
+
+  // Test 5: error on invalid yaml on command line
+  auto Style5 = getStyle("{invalid_key=invalid_value}", "a.h", "LLVM", "", );
+  ASSERT_FALSE((bool)Style5);
+  llvm::consumeError(Style5.takeError());
+
+  // Test 6: error on invalid style
+  auto Style6 = getStyle("KungFu", "a.h", "LLVM", "", );
+  ASSERT_FALSE((bool)Style6);
+  llvm::consumeError(Style6.takeError());
+
+  // Test 7: found config file, error on parsing it
+  ASSERT_TRUE(
+  FS.addFile("/d/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM\n"
+  "InvalidKey: InvalidValue")));
+  ASSERT_TRUE(
+  FS.addFile("/d/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style7 = getStyle("file", "/d/.clang-format", "LLVM", "", );
+  ASSERT_FALSE((bool)Style7);
+  llvm::consumeError(Style7.takeError());
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: tools/clang-format/ClangFormat.cpp
===
--- tools/clang-format/ClangFormat.cpp
+++ tools/clang-format/ClangFormat.cpp
@@ -249,12 +249,17 @@
   if (fillRanges(Code.get(), Ranges))
 return true;
   StringRef AssumedFileName = (FileName == "-") ? AssumeFileName : FileName;
-  FormatStyle FormatStyle =
+
+  llvm::Expected FormatStyle =
   getStyle(Style, AssumedFileName, FallbackStyle, Code->getBuffer());
+  if (!FormatStyle) {
+llvm::errs() << llvm::toString(FormatStyle.takeError()) << "\n";
+return true;
+  }
   if (SortIncludes.getNumOccurrences() != 0)
-FormatStyle.SortIncludes = SortIncludes;
+FormatStyle->SortIncludes = SortIncludes;
   unsigned CursorPosition = Cursor;
-  Replacements Replaces = sortIncludes(FormatStyle, Code->getBuffer(), Ranges,
+  Replacements Replaces = 

[PATCH] D27440: clang-format-vsix: fail when clang-format outputs to stderr

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D27440#634043, @cameron314 wrote:

> >> Thanks, I'll check these out! Btw, I noticed that the clang-format tests 
> >> are non-Windows due to path assumptions. Is this a lost cause, or just 
> >> something no one's bothered to look into yet?
> > 
> > No one's bothered looking into it yet.
>
> Which tests? We run the unit tests (FormatTests.exe) locally just fine on 
> Windows.


There was one test disabled for MSVC, which I fixed and enabled here: 
https://reviews.llvm.org/D27971

I will soon close this issue (https://reviews.llvm.org/D27440) once 
https://reviews.llvm.org/D28081 goes through as clang-format should return 
non-zero when an error occurs.

In https://reviews.llvm.org/D27440#634043, @cameron314 wrote:

> >> Thanks, I'll check these out! Btw, I noticed that the clang-format tests 
> >> are non-Windows due to path assumptions. Is this a lost cause, or just 
> >> something no one's bothered to look into yet?
> > 
> > No one's bothered looking into it yet.
>
> Which tests? We run the unit tests (FormatTests.exe) locally just fine on 
> Windows.





https://reviews.llvm.org/D27440



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano updated this revision to Diff 82831.
amaiorano added a comment.

More changes as suggested by @ioeric. I asked a question earlier about 
clang-tools-extras and understanding how not to break the build bots. If you 
can shed some light on that, I'd appreciate it, thanks :)


https://reviews.llvm.org/D28081

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Tooling/Refactoring.cpp
  tools/clang-format/ClangFormat.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestObjC.cpp

Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -69,16 +69,20 @@
 };
 
 TEST_F(FormatTestObjC, DetectsObjCInHeaders) {
-  Style = getStyle("LLVM", "a.h", "none", "@interface\n"
-  "- (id)init;");
-  EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
-  Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+  auto Style1 = getStyle("LLVM", "a.h", "none", "@interface\n"
+   "- (id)init;");
+  ASSERT_TRUE((bool)Style1);
+  EXPECT_EQ(FormatStyle::LK_ObjC, Style1->Language);
+
+  auto Style2 = getStyle("LLVM", "a.h", "none", "@interface\n"
   "+ (id)init;");
-  EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
+  ASSERT_TRUE((bool)Style2);
+  EXPECT_EQ(FormatStyle::LK_ObjC, Style2->Language);
 
   // No recognizable ObjC.
-  Style = getStyle("LLVM", "a.h", "none", "void f() {}");
-  EXPECT_EQ(FormatStyle::LK_Cpp, Style.Language);
+  auto Style3 = getStyle("LLVM", "a.h", "none", "void f() {}");
+  ASSERT_TRUE((bool)Style3);
+  EXPECT_EQ(FormatStyle::LK_Cpp, Style3->Language);
 }
 
 TEST_F(FormatTestObjC, FormatObjCTryCatch) {
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10945,22 +10945,51 @@
   ASSERT_TRUE(
   FS.addFile("/a/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style1 = getStyle("file", "/a/.clang-format", "Google", "", );
-  ASSERT_EQ(Style1, getLLVMStyle());
+  ASSERT_TRUE((bool)Style1);
+  ASSERT_EQ(*Style1, getLLVMStyle());
 
   // Test 2: fallback to default.
   ASSERT_TRUE(
   FS.addFile("/b/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style2 = getStyle("file", "/b/test.cpp", "Mozilla", "", );
-  ASSERT_EQ(Style2, getMozillaStyle());
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getMozillaStyle());
 
   // Test 3: format file in parent directory.
   ASSERT_TRUE(
   FS.addFile("/c/.clang-format", 0,
  llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
   ASSERT_TRUE(FS.addFile("/c/sub/sub/sub/test.cpp", 0,
  llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style3 = getStyle("file", "/c/sub/sub/sub/test.cpp", "LLVM", "", );
-  ASSERT_EQ(Style3, getGoogleStyle());
+  ASSERT_TRUE((bool)Style3);
+  ASSERT_EQ(*Style3, getGoogleStyle());
+
+  // Test 4: error on invalid fallback style
+  auto Style4 = getStyle("file", "a.h", "KungFu", "", );
+  ASSERT_FALSE((bool)Style4);
+  llvm::consumeError(Style4.takeError());
+
+  // Test 5: error on invalid yaml on command line
+  auto Style5 = getStyle("{invalid_key=invalid_value}", "a.h", "LLVM", "", );
+  ASSERT_FALSE((bool)Style5);
+  llvm::consumeError(Style5.takeError());
+
+  // Test 6: error on invalid style
+  auto Style6 = getStyle("KungFu", "a.h", "LLVM", "", );
+  ASSERT_FALSE((bool)Style6);
+  llvm::consumeError(Style6.takeError());
+
+  // Test 7: found config file, error on parsing it
+  ASSERT_TRUE(
+  FS.addFile("/d/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM\n"
+  "InvalidKey: InvalidValue")));
+  ASSERT_TRUE(
+  FS.addFile("/d/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style7 = getStyle("file", "/d/.clang-format", "LLVM", "", );
+  ASSERT_FALSE((bool)Style7);
+  llvm::consumeError(Style7.takeError());
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: tools/clang-format/ClangFormat.cpp
===
--- tools/clang-format/ClangFormat.cpp
+++ tools/clang-format/ClangFormat.cpp
@@ -249,8 +249,14 @@
   if (fillRanges(Code.get(), Ranges))
 return true;
   StringRef AssumedFileName = (FileName == "-") ? AssumeFileName : FileName;
-  FormatStyle FormatStyle =
+
+  llvm::Expected FormatStyleOrError =
   getStyle(Style, AssumedFileName, FallbackStyle, Code->getBuffer());
+  if (!FormatStyleOrError) {
+llvm::errs() << llvm::toString(FormatStyleOrError.takeError()) << "\n";
+return true;
+  }
+  FormatStyle FormatStyle = *FormatStyleOrError;
   if (SortIncludes.getNumOccurrences() != 0)
 FormatStyle.SortIncludes = 

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added inline comments.



Comment at: unittests/Format/FormatTestObjC.cpp:72
 TEST_F(FormatTestObjC, DetectsObjCInHeaders) {
-  Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+  Style = *getStyle("LLVM", "a.h", "none", "@interface\n"
   "- (id)init;");

amaiorano wrote:
> ioeric wrote:
> > amaiorano wrote:
> > > In these tests, I'm assuming getStyle returns a valid FormatSyle. I could 
> > > add the same types of validation as in the FormatStyle.GetStyleOfFile 
> > > tests.
> > Please add proper checking as above for returned values.
> Hmm, so I could replace the Style member of the fixture class with 
> Expected, and then change all "Style." with "Style->" in the 
> rest of the test file, or only in this specific test, I could store the 
> result in a local Expected, check that it's valid, and then 
> assign to Style. The latter is simpler; only question I have is how to name 
> the local variable - can I go with StyleOrError? Style2?
Another option here is to make this a non-fixture TEST and just declare a local 
Expected Style for this specific test, which would work fine.


https://reviews.llvm.org/D28081



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D28081#633521, @ioeric wrote:

> Some nits. Some is almost good :)
>
> BTW, do you have clang-tools-extra in your source tree? There are also some 
> references in the subtree to the changed interface. It would be nice if you 
> could also fix them in a separate patch and commit these two patches together 
> (I mean, within a short period of time) so that you wouldn't break build bots.
>
> References should be found in these files:
>
>   extra/change-namespace/ChangeNamespace.cpp
>   extra/clang-move/ClangMove.cpp
>   extra/include-fixer/tool/ClangIncludeFixer.cpp   
>   extra/clang-apply-replacements/tool/ClangApplyReplacementsMain.cpp
>   extra/clang-tidy/ClangTidy.cpp
>
>
> Thanks!


I'll grab clang-tools-extras and make the second patch as you suggest. Btw, can 
you explain how I would avoid breaking build bots? I assume you mean that 
clang-tools-extras gets built separately against some version of clang, which 
gets auto-updated. When would I know the right time to push the second patch 
through?

Also, I assume I'd have to get this second patch approved before ever pushing 
the first, right?




Comment at: lib/Format/Format.cpp:424
 
+llvm::Error make_string_error(const llvm::Twine ) {
+  return llvm::make_error(Message,

ioeric wrote:
> Maybe make this `inline`?
Yes.



Comment at: lib/Format/Format.cpp:1901
+  // FIXME: If FallbackStyle is explicitly "none", this effectively disables
+  // replacements. Fix this by setting a separate FormatStyle variable and
+  // returning it when we mean to return the fallback style explicitly.

ioeric wrote:
> I'd state the problem with a specific solution only if I am sure it is the 
> best solution.
Good point, will remove the solution.



Comment at: lib/Tooling/Refactoring.cpp:86
 
-format::FormatStyle CurStyle = format::getStyle(Style, FilePath, "LLVM");
+llvm::Expected FormatStyleOrError =
+format::getStyle(Style, FilePath, "LLVM");

ioeric wrote:
> There is a `NewReplacements` below which is also `llvm::Expected`. 
> `FormatStyleOrError` is  a fine name, but to be we have been naming 
> `Expected` types without `OrError` postfix, so I'd go without `OrError` 
> postfix for consistency append `OrError` postfix to other expected variables. 
> Personally, I find expected variables without `OrError` postfix easier to 
> understand, especially in error checking. For example, `if 
> (!FormatStyleOrError)` is a bit awkward to read while `if (!FormatStyle)` is 
> more straight-forward IMO.
> 
> Same for other changes.
Agreed. For consistency, would you prefer I also use 'auto' for the return type 
rather than llvm::Expected as is done for NewReplacements?



Comment at: unittests/Format/FormatTest.cpp:10972
+  auto ErrorMsg4 = llvm::toString(Style4.takeError());
+  ASSERT_GT(ErrorMsg4.length(), 0);
+

ioeric wrote:
> This is a bit strange... Just `llvm::consumeError(Style.takeError())` if you 
> don't bother to check the error message, e.g. with 
> `llvm::StringRef::starswith` or RE match.
Yeah, I'll go with the consumeError.



Comment at: unittests/Format/FormatTestObjC.cpp:72
 TEST_F(FormatTestObjC, DetectsObjCInHeaders) {
-  Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+  Style = *getStyle("LLVM", "a.h", "none", "@interface\n"
   "- (id)init;");

ioeric wrote:
> amaiorano wrote:
> > In these tests, I'm assuming getStyle returns a valid FormatSyle. I could 
> > add the same types of validation as in the FormatStyle.GetStyleOfFile tests.
> Please add proper checking as above for returned values.
Hmm, so I could replace the Style member of the fixture class with 
Expected, and then change all "Style." with "Style->" in the rest 
of the test file, or only in this specific test, I could store the result in a 
local Expected, check that it's valid, and then assign to Style. 
The latter is simpler; only question I have is how to name the local variable - 
can I go with StyleOrError? Style2?


https://reviews.llvm.org/D28081



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano updated this revision to Diff 82814.
amaiorano added a comment.

Reverted the FallbackStyle code and added a FIXME as @ioeric suggested. I'll 
fix the fallback style "none" bug in a separate change.


https://reviews.llvm.org/D28081

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Tooling/Refactoring.cpp
  tools/clang-format/ClangFormat.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestObjC.cpp

Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -69,15 +69,15 @@
 };
 
 TEST_F(FormatTestObjC, DetectsObjCInHeaders) {
-  Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+  Style = *getStyle("LLVM", "a.h", "none", "@interface\n"
   "- (id)init;");
   EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
-  Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+  Style = *getStyle("LLVM", "a.h", "none", "@interface\n"
   "+ (id)init;");
   EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
 
   // No recognizable ObjC.
-  Style = getStyle("LLVM", "a.h", "none", "void f() {}");
+  Style = *getStyle("LLVM", "a.h", "none", "void f() {}");
   EXPECT_EQ(FormatStyle::LK_Cpp, Style.Language);
 }
 
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10945,22 +10945,55 @@
   ASSERT_TRUE(
   FS.addFile("/a/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style1 = getStyle("file", "/a/.clang-format", "Google", "", );
-  ASSERT_EQ(Style1, getLLVMStyle());
+  ASSERT_TRUE((bool)Style1);
+  ASSERT_EQ(*Style1, getLLVMStyle());
 
   // Test 2: fallback to default.
   ASSERT_TRUE(
   FS.addFile("/b/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style2 = getStyle("file", "/b/test.cpp", "Mozilla", "", );
-  ASSERT_EQ(Style2, getMozillaStyle());
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getMozillaStyle());
 
   // Test 3: format file in parent directory.
   ASSERT_TRUE(
   FS.addFile("/c/.clang-format", 0,
  llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
   ASSERT_TRUE(FS.addFile("/c/sub/sub/sub/test.cpp", 0,
  llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style3 = getStyle("file", "/c/sub/sub/sub/test.cpp", "LLVM", "", );
-  ASSERT_EQ(Style3, getGoogleStyle());
+  ASSERT_TRUE((bool)Style3);
+  ASSERT_EQ(*Style3, getGoogleStyle());
+
+  // Test 4: error on invalid fallback style
+  auto Style4 = getStyle("file", "a.h", "KungFu", "", );
+  ASSERT_FALSE((bool)Style4);
+  auto ErrorMsg4 = llvm::toString(Style4.takeError());
+  ASSERT_GT(ErrorMsg4.length(), 0);
+
+  // Test 5: error on invalid yaml on command line
+  auto Style5 = getStyle("{invalid_key=invalid_value}", "a.h", "LLVM", "", );
+  ASSERT_FALSE((bool)Style5);
+  auto ErrorMsg5 = llvm::toString(Style5.takeError());
+  ASSERT_GT(ErrorMsg5.length(), 0);
+
+  // Test 6: error on invalid style
+  auto Style6 = getStyle("KungFu", "a.h", "LLVM", "", );
+  ASSERT_FALSE((bool)Style6);
+  auto ErrorMsg6 = llvm::toString(Style6.takeError());
+  ASSERT_GT(ErrorMsg6.length(), 0);
+
+  // Test 7: found config file, error on parsing it
+  ASSERT_TRUE(
+  FS.addFile("/d/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM\n"
+  "InvalidKey: InvalidValue")));
+  ASSERT_TRUE(
+  FS.addFile("/d/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style7 = getStyle("file", "/d/.clang-format", "LLVM", "", );
+  ASSERT_FALSE((bool)Style7);
+  auto ErrorMsg7 = llvm::toString(Style7.takeError());
+  ASSERT_GT(ErrorMsg7.length(), 0);
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: tools/clang-format/ClangFormat.cpp
===
--- tools/clang-format/ClangFormat.cpp
+++ tools/clang-format/ClangFormat.cpp
@@ -249,8 +249,14 @@
   if (fillRanges(Code.get(), Ranges))
 return true;
   StringRef AssumedFileName = (FileName == "-") ? AssumeFileName : FileName;
-  FormatStyle FormatStyle =
+
+  llvm::Expected FormatStyleOrError =
   getStyle(Style, AssumedFileName, FallbackStyle, Code->getBuffer());
+  if (!FormatStyleOrError) {
+llvm::errs() << llvm::toString(FormatStyleOrError.takeError()) << "\n";
+return true;
+  }
+  FormatStyle FormatStyle = *FormatStyleOrError;
   if (SortIncludes.getNumOccurrences() != 0)
 FormatStyle.SortIncludes = SortIncludes;
   unsigned CursorPosition = Cursor;
@@ -334,10 +340,16 @@
 cl::PrintHelpMessage();
 
   if (DumpConfig) {
-std::string Config =
-clang::format::configurationAsText(clang::format::getStyle(
+llvm::Expected 

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added inline comments.



Comment at: lib/Format/Format.cpp:1900
 
-  if (!getPredefinedStyle(FallbackStyle, Style.Language, )) {
-llvm::errs() << "Invalid fallback style \"" << FallbackStyle
- << "\" using LLVM style\n";
-return Style;
-  }
+  FormatStyle FallbackStyle = getNoStyle();
+  if (!getPredefinedStyle(FallbackStyleName, Style.Language, ))

amaiorano wrote:
> Going over my diff, I realize that I didn't revert this local FormatStyle 
> instance to make use of the top-level declared "FormatStyle Style" local 
> variable. I will revert this one too.
I wrote the above comment before realizing there was a bug when fallback style 
is "none". As I explained in my long comment, we may actually want to keep this 
as a separate variable to fix the bug and ensure fallback style is only ever 
returned when no config is found, which is exactly what my change currently 
does.


https://reviews.llvm.org/D28081



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

Hello everyone, so after a few more tests, I've uncovered a bug and perhaps a 
different meaning for fallback style. First, the bug: if you set fallback style 
to "none", clang-format will perform no replacements. This happens because 
getStyle will first initialize its local Style variable to LLVM style, and then 
because a fallback style is set, will then set it to the "none" style, will 
ends up setting Style.DisableFormatting to true. After that, when we parse YAML 
(either from Style arg or a config file), we use the Style variable as the 
"template" for fields that haven't been set. In this case, the "none" fallback 
style causes DisableFormatting to remain true, so no formatting will take place.

As it happens, my first diff patch uploaded here fixed this issue by accident. 
Instead of reusing the same local Style variable, I declared one for each case 
where we'd need to parse. The fallback style case would use its own variable, 
FallbackStyle, which would not be used as the template style when parsing the 
YAML config.

What's interesting is that the way the code is originally written allows you to 
use fallback style as a way to set the "base" configuration for which the 
subsequently parsed YAML overlays. For example, if I don't set fallback style, 
the assumed base style is "LLVM", and any YAML parsed modifies this LLVM base 
style. But if I pass a fallback style of "Mozilla", then this becomes the base 
style over which the YAML overlays.

So to my mind, we have 2 approaches to fix the "none" style bug:

1. Go with a similar approach to what I did originally; that is, we always 
assume LLVM as the base style, and make sure that the fallback style is not 
used as the base style, but rather only as the style to return if none is 
found. I think this is what FallbackStyle was originally intended for.

2. Allow fallback style to maintain its current meaning - that is, as a way to 
set the base style when "style" is "file" or YAML. In this case, I believe the 
right thing is to treat FallbackStyle set to "none" as though no fallback style 
were passed in at all. Concretely, we might want t to modify getPredefinedStyle 
to return LLVM style when "none" is passed in, instead of what it does now. I 
personally think this is more confusing, and also introduces more risk.

Let me know what you think. If we go with option 1, I could fold the fix into 
this change.




Comment at: lib/Format/Format.cpp:1900
 
-  if (!getPredefinedStyle(FallbackStyle, Style.Language, )) {
-llvm::errs() << "Invalid fallback style \"" << FallbackStyle
- << "\" using LLVM style\n";
-return Style;
-  }
+  FormatStyle FallbackStyle = getNoStyle();
+  if (!getPredefinedStyle(FallbackStyleName, Style.Language, ))

Going over my diff, I realize that I didn't revert this local FormatStyle 
instance to make use of the top-level declared "FormatStyle Style" local 
variable. I will revert this one too.


https://reviews.llvm.org/D28081



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano updated this revision to Diff 82708.
amaiorano added a comment.

Updated with the changes @ioeric suggested.


https://reviews.llvm.org/D28081

Files:
  include/clang/Format/Format.h
  lib/Format/Format.cpp
  lib/Tooling/Refactoring.cpp
  tools/clang-format/ClangFormat.cpp
  unittests/Format/FormatTest.cpp
  unittests/Format/FormatTestObjC.cpp

Index: unittests/Format/FormatTestObjC.cpp
===
--- unittests/Format/FormatTestObjC.cpp
+++ unittests/Format/FormatTestObjC.cpp
@@ -69,15 +69,15 @@
 };
 
 TEST_F(FormatTestObjC, DetectsObjCInHeaders) {
-  Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+  Style = *getStyle("LLVM", "a.h", "none", "@interface\n"
   "- (id)init;");
   EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
-  Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+  Style = *getStyle("LLVM", "a.h", "none", "@interface\n"
   "+ (id)init;");
   EXPECT_EQ(FormatStyle::LK_ObjC, Style.Language);
 
   // No recognizable ObjC.
-  Style = getStyle("LLVM", "a.h", "none", "void f() {}");
+  Style = *getStyle("LLVM", "a.h", "none", "void f() {}");
   EXPECT_EQ(FormatStyle::LK_Cpp, Style.Language);
 }
 
Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10945,22 +10945,55 @@
   ASSERT_TRUE(
   FS.addFile("/a/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style1 = getStyle("file", "/a/.clang-format", "Google", "", );
-  ASSERT_EQ(Style1, getLLVMStyle());
+  ASSERT_TRUE((bool)Style1);
+  ASSERT_EQ(*Style1, getLLVMStyle());
 
   // Test 2: fallback to default.
   ASSERT_TRUE(
   FS.addFile("/b/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style2 = getStyle("file", "/b/test.cpp", "Mozilla", "", );
-  ASSERT_EQ(Style2, getMozillaStyle());
+  ASSERT_TRUE((bool)Style2);
+  ASSERT_EQ(*Style2, getMozillaStyle());
 
   // Test 3: format file in parent directory.
   ASSERT_TRUE(
   FS.addFile("/c/.clang-format", 0,
  llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: Google")));
   ASSERT_TRUE(FS.addFile("/c/sub/sub/sub/test.cpp", 0,
  llvm::MemoryBuffer::getMemBuffer("int i;")));
   auto Style3 = getStyle("file", "/c/sub/sub/sub/test.cpp", "LLVM", "", );
-  ASSERT_EQ(Style3, getGoogleStyle());
+  ASSERT_TRUE((bool)Style3);
+  ASSERT_EQ(*Style3, getGoogleStyle());
+
+  // Test 4: error on invalid fallback style
+  auto Style4 = getStyle("file", "a.h", "KungFu", "", );
+  ASSERT_FALSE((bool)Style4);
+  auto ErrorMsg4 = llvm::toString(Style4.takeError());
+  ASSERT_GT(ErrorMsg4.length(), 0);
+
+  // Test 5: error on invalid yaml on command line
+  auto Style5 = getStyle("{invalid_key=invalid_value}", "a.h", "LLVM", "", );
+  ASSERT_FALSE((bool)Style5);
+  auto ErrorMsg5 = llvm::toString(Style5.takeError());
+  ASSERT_GT(ErrorMsg5.length(), 0);
+
+  // Test 6: error on invalid style
+  auto Style6 = getStyle("KungFu", "a.h", "LLVM", "", );
+  ASSERT_FALSE((bool)Style6);
+  auto ErrorMsg6 = llvm::toString(Style6.takeError());
+  ASSERT_GT(ErrorMsg6.length(), 0);
+
+  // Test 7: found config file, error on parsing it
+  ASSERT_TRUE(
+  FS.addFile("/d/.clang-format", 0,
+ llvm::MemoryBuffer::getMemBuffer("BasedOnStyle: LLVM\n"
+  "InvalidKey: InvalidValue")));
+  ASSERT_TRUE(
+  FS.addFile("/d/test.cpp", 0, llvm::MemoryBuffer::getMemBuffer("int i;")));
+  auto Style7 = getStyle("file", "/d/.clang-format", "LLVM", "", );
+  ASSERT_FALSE((bool)Style7);
+  auto ErrorMsg7 = llvm::toString(Style7.takeError());
+  ASSERT_GT(ErrorMsg7.length(), 0);
 }
 
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
Index: tools/clang-format/ClangFormat.cpp
===
--- tools/clang-format/ClangFormat.cpp
+++ tools/clang-format/ClangFormat.cpp
@@ -249,8 +249,14 @@
   if (fillRanges(Code.get(), Ranges))
 return true;
   StringRef AssumedFileName = (FileName == "-") ? AssumeFileName : FileName;
-  FormatStyle FormatStyle =
+
+  llvm::Expected FormatStyleOrError =
   getStyle(Style, AssumedFileName, FallbackStyle, Code->getBuffer());
+  if (!FormatStyleOrError) {
+llvm::errs() << llvm::toString(FormatStyleOrError.takeError()) << "\n";
+return true;
+  }
+  FormatStyle FormatStyle = *FormatStyleOrError;
   if (SortIncludes.getNumOccurrences() != 0)
 FormatStyle.SortIncludes = SortIncludes;
   unsigned CursorPosition = Cursor;
@@ -334,10 +340,16 @@
 cl::PrintHelpMessage();
 
   if (DumpConfig) {
-std::string Config =
-clang::format::configurationAsText(clang::format::getStyle(
+llvm::Expected FormatStyleOrError =
+clang::format::getStyle(
 Style, FileNames.empty() ? 

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added inline comments.



Comment at: lib/Format/Format.cpp:1984
+// If so, can't return this error here...
+return make_string_error("Configuration file(s) do(es) not support " +
+ getLanguageName(Language) + ": " +

amaiorano wrote:
> See the TODO comment above (which will be removed obviously). Is it an error 
> if we find no suitable config for the input language? If that happens, should 
> the fallback style be returned?
@ioeric Do you have any thoughts on my question here? Say the user specified 
"-file" and a fallback style, and we find files but they are not suitable (for 
a different language), do we use the fallback style, since it's as if we found 
no config file. If so, then we wouldn't consider this an error, and therefore 
would not print nor return the message that we see here ("Configuration file(s) 
do(es) not support...").

The fact that the original code output to errs() here leads me to believe that 
this should be considered an error condition, in which case I should keep my 
change - that is, return an error here and _not_ return the fallback style.


https://reviews.llvm.org/D28081



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

Thank you, @ioeric for your feedback!




Comment at: include/clang/Format/Format.h:862
 ///
 /// \returns FormatStyle as specified by ``StyleName``. If no style could be
 /// determined, the default is LLVM Style (see ``getLLVMStyle()``).

ioeric wrote:
> Is this still true?
No, it's not true, and I intend to update this comment. Thanks for pointing it 
out :)



Comment at: lib/Format/Format.cpp:424
 
+template  llvm::Error make_string_error(ArgTs &&... Args) {
+  return llvm::make_error(std::forward(Args)...,

ioeric wrote:
> Why do you need a template if you only use this function with one argument?
In the end, I didn't need it. But since make_error accepts variable args, I 
figured this was more flexible. I can simplify it in the end, I only pass a 
single param.



Comment at: lib/Format/Format.cpp:1890
   }
-  FormatStyle Style = getLLVMStyle();
-  Style.Language = getLanguageByFileName(FileName);
+  FormatStyle::LanguageKind Language = getLanguageByFileName(FileName);
 

ioeric wrote:
> amaiorano wrote:
> > Since we won't always return a FormatStyle, we no longer construct one 
> > here. Rather, instances are created in local scopes below when required.
> I'd probably keep the default style so that we don't need to set `Language` 
> repeatedly below.
The thing is, it made a lot of sense to have a default constructed FormatStyle 
before when we were sure to always return it. Since now we only return a Style 
under 2 specific conditions (found a config file, or did not return the 
FallbackStyle), otherwise return an error, I feel like it's more clear this 
way. But if you firmly disagree, I can go back.

To make returning the Style with current Language more clear, I could use a 
function that ties the two together and returns it (perhaps a local lambda), 
like StyleWithLanguage(Style, Language).



Comment at: lib/Format/Format.cpp:1900
+  FormatStyle FallbackStyle = getNoStyle();
+  if (!getPredefinedStyle(FallbackStyleName, Language, )) {
+return make_string_error("Invalid fallback style \"" +

ioeric wrote:
> Nit: prefer no curly brace around one liners. Same below.
Will do.



Comment at: lib/Format/Format.cpp:1901
+  if (!getPredefinedStyle(FallbackStyleName, Language, )) {
+return make_string_error("Invalid fallback style \"" +
+ FallbackStyleName.str());

ioeric wrote:
> amaiorano wrote:
> > I am unsure whether I should be returning these error strings at all. I say 
> > this because some of the functions called in this one will output to 
> > errs(), which means the caller doesn't get the full picture of what went 
> > wrong.
> > 
> > Maybe it's better to keep the original code that outputs to errs(), and 
> > just return an error code instead. Thoughts?
> I think returning an `Expected` is the right approach. I think we should 
> consider using customized format-relayed error codes (like that in the 
> `tooling::Replacements` library) to provide richer information. For example, 
> you could use customized error codes instead of 
> `llvm::inconvertibleErrorCode` when constructing a `StringError` to provide 
> additional information besides the error message. Other interfaces in the 
> format library can potentially benefit from codes as well (e.g. `ParseError` 
> can be merged). 
I agree with you, returning more specific error codes would be useful. However, 
I'm thinking we go at it incrementally. The point of this change is to have 
getStyle return when an error occurs so that we return non-zero from main, and 
more importantly, not use the fallback style when this happens. 

In that respect, what I'm wondering is whether I should just leave the errs() 
output in getStyle, and simply return an error code, or keep what I've done 
(returning StringError) as a stepping stone in the right direction - that is, 
eventually returning customized format-relayed error codes, as you say.




Comment at: lib/Tooling/Refactoring.cpp:86
 
-format::FormatStyle CurStyle = format::getStyle(Style, FilePath, "LLVM");
+   llvm::Expected FormatStyleOrError = 
format::getStyle(Style, FilePath, "LLVM");
+   if (!FormatStyleOrError) {

ioeric wrote:
> Is this indentation intended? 
No. I will definitely do a formatting pass. I'm using Visual Studio with the 
clang-format extension, which works alright, but Visual Studio fights against 
it with its own formatting. Indeed, the reason I even started contributing to 
clang-format was because I wanted to improve the VS extension. I've already 
pushed a few changes to it, and intend to do a little more to make the 
experience simpler. In any case, my next diff upload will have proper 
formatting everywhere.


https://reviews.llvm.org/D28081



___
cfe-commits 

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

One more thing I forgot to mention is that this change comes from a discussion 
we had on this other change: https://reviews.llvm.org/D27440 where @djasper 
agreed that "fallback-style should only be used when there is no .clang-format 
file. If we find one, and it doesn't parse correctly, we should neither use the 
fallback style nor scan in higher-level directories (not sure whether we 
currently do that).".


https://reviews.llvm.org/D28081



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D28081: Make GetStyle return Expected instead of FormatStyle

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added inline comments.



Comment at: unittests/Format/FormatTestObjC.cpp:72
 TEST_F(FormatTestObjC, DetectsObjCInHeaders) {
-  Style = getStyle("LLVM", "a.h", "none", "@interface\n"
+  Style = *getStyle("LLVM", "a.h", "none", "@interface\n"
   "- (id)init;");

In these tests, I'm assuming getStyle returns a valid FormatSyle. I could add 
the same types of validation as in the FormatStyle.GetStyleOfFile tests.


https://reviews.llvm.org/D28081



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27971: Make FormatStyle.GetStyleOfFile test work on MSVC

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added inline comments.



Comment at: lib/Format/Format.cpp:1920-1922
+  std::error_code EC = FS->makeAbsolute(Path);
+  assert(!EC);
+  (void)EC;

klimek wrote:
> I think if makeAbsolute doesn't work, we will probably want to err out here:
> if (EC) {
>   llvm::errs() << ... << EC.message << ...
>   return Style;
> }
Okay. I based myself on the fact that the old code didn't do error checking on 
fs::make_absolute - in fact most calls to fs::make_absolute in the codebase 
assert on the result. I suppose it's because it's just a string manipulation 
function, and if it fails, the subsequent code that will make use of the path 
will fail with a more useful error.

In any case, I don't mind handling the error here as you propose.



https://reviews.llvm.org/D27971



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27971: Make FormatStyle.GetStyleOfFile test work on MSVC

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano updated this revision to Diff 82185.
amaiorano added a comment.

As agreed, modified getStyle to make use of vfs::FileSystem::makeAbsolute. I 
would modify the body of the commit as follows:

Modify getStyle to use vfs::FileSystem::makeAbsolute just like FS.addFile does,
rather than sys::fs::make_absolute. The latter gets the CWD from the platform,
while the former expects it to be set by the client, causing a mismatch when
converting relative paths to absolute.

(not sure if I should have just modified the Summary section of this 
Phabricator review)


https://reviews.llvm.org/D27971

Files:
  lib/Format/Format.cpp
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10907,10 +10907,6 @@
 format("auto a = unique_ptr < Foo < Bar>[10]> ;", Spaces));
 }
 
-// Since this test case uses UNIX-style file path. We disable it for MS
-// compiler.
-#if !defined(_MSC_VER) && !defined(__MINGW32__)
-
 TEST(FormatStyle, GetStyleOfFile) {
   vfs::InMemoryFileSystem FS;
   // Test 1: format file in the same directory.
@@ -10938,8 +10934,6 @@
   ASSERT_EQ(Style3, getGoogleStyle());
 }
 
-#endif // _MSC_VER
-
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
   // Column limit is 20.
   std::string Code = "Type *a =\n"
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -1917,7 +1917,10 @@
   // Look for .clang-format/_clang-format file in the file's parent 
directories.
   SmallString<128> UnsuitableConfigFiles;
   SmallString<128> Path(FileName);
-  llvm::sys::fs::make_absolute(Path);
+  std::error_code EC = FS->makeAbsolute(Path);
+  assert(!EC);
+  (void)EC;
+
   for (StringRef Directory = Path; !Directory.empty();
Directory = llvm::sys::path::parent_path(Directory)) {
 


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10907,10 +10907,6 @@
 format("auto a = unique_ptr < Foo < Bar>[10]> ;", Spaces));
 }
 
-// Since this test case uses UNIX-style file path. We disable it for MS
-// compiler.
-#if !defined(_MSC_VER) && !defined(__MINGW32__)
-
 TEST(FormatStyle, GetStyleOfFile) {
   vfs::InMemoryFileSystem FS;
   // Test 1: format file in the same directory.
@@ -10938,8 +10934,6 @@
   ASSERT_EQ(Style3, getGoogleStyle());
 }
 
-#endif // _MSC_VER
-
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
   // Column limit is 20.
   std::string Code = "Type *a =\n"
Index: lib/Format/Format.cpp
===
--- lib/Format/Format.cpp
+++ lib/Format/Format.cpp
@@ -1917,7 +1917,10 @@
   // Look for .clang-format/_clang-format file in the file's parent directories.
   SmallString<128> UnsuitableConfigFiles;
   SmallString<128> Path(FileName);
-  llvm::sys::fs::make_absolute(Path);
+  std::error_code EC = FS->makeAbsolute(Path);
+  assert(!EC);
+  (void)EC;
+
   for (StringRef Directory = Path; !Directory.empty();
Directory = llvm::sys::path::parent_path(Directory)) {
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27971: Make FormatStyle.GetStyleOfFile test work on MSVC

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D27971#627543, @klimek wrote:

> Why isn't the right solution to make getStyle() use vfs::FileSystem? 
> Generally, everything in clang-format (well, in clang) should use 
> vfs::FileSystem for file access.


You're absolutely right, this is the right solution. I was under the false 
impression that llvm::fs was preferred. What threw me off was that 
vfs::FileSystem::status() returns llvm::fs::file_type::* values.

I already made the change locally to getStyle() to use vfs::FileSystem and it 
works great. Will update the patch asap. Thanks!


https://reviews.llvm.org/D27971



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27971: Make FormatStyle.GetStyleOfFile test work on MSVC

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano created this revision.
amaiorano added reviewers: klimek, djasper, hans, cfe-commits.

This is more a workaround than a real fix. The real problem is that FS.addFile 
uses clang::vfs::FileSystem::makeAbsolute to convert the input path to an 
absolute path, while getStyle uses llvm::sys::fs::make_absolute, which while 
named similarly,  behave differently. Both will join the input path to the 
current working directory (CWD), except that in the former case, you need to 
have set the CWD explicitly via 
clang::vfs::FileSystem::setCurrentWorkingDirectory, while the latter retrieves 
the CWD from the platform abstraction (llvm::sys::fs::current_path).

A better fix might be to make clang::vfs::FileSystem match the behaviour of 
llvm::sys::fs, having it retrieve the CWD from the platform, rather than having 
the client set it explicitly. Or perhaps clang::vfs::FileSystem should be 
rewritten in terms of llvm::sys::fs, and deprecate the former so that code 
moves towards using the latter.


https://reviews.llvm.org/D27971

Files:
  unittests/Format/FormatTest.cpp


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10907,12 +10907,16 @@
 format("auto a = unique_ptr < Foo < Bar>[10]> ;", Spaces));
 }
 
-// Since this test case uses UNIX-style file path. We disable it for MS
-// compiler.
-#if !defined(_MSC_VER) && !defined(__MINGW32__)
-
 TEST(FormatStyle, GetStyleOfFile) {
   vfs::InMemoryFileSystem FS;
+
+  // Set CWD so that clang::vfs::FileSystem::makeAbsolute and
+  // llvm::sys::fs::make_absolute return the same thing.
+  llvm::SmallString<128> InitialDirectory;
+  std::error_code EC = llvm::sys::fs::current_path(InitialDirectory);
+  EXPECT_EQ(EC.value(), 0);
+  FS.setCurrentWorkingDirectory(InitialDirectory);
+
   // Test 1: format file in the same directory.
   ASSERT_TRUE(
   FS.addFile("/a/.clang-format", 0,
@@ -10938,8 +10942,6 @@
   ASSERT_EQ(Style3, getGoogleStyle());
 }
 
-#endif // _MSC_VER
-
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
   // Column limit is 20.
   std::string Code = "Type *a =\n"


Index: unittests/Format/FormatTest.cpp
===
--- unittests/Format/FormatTest.cpp
+++ unittests/Format/FormatTest.cpp
@@ -10907,12 +10907,16 @@
 format("auto a = unique_ptr < Foo < Bar>[10]> ;", Spaces));
 }
 
-// Since this test case uses UNIX-style file path. We disable it for MS
-// compiler.
-#if !defined(_MSC_VER) && !defined(__MINGW32__)
-
 TEST(FormatStyle, GetStyleOfFile) {
   vfs::InMemoryFileSystem FS;
+
+  // Set CWD so that clang::vfs::FileSystem::makeAbsolute and
+  // llvm::sys::fs::make_absolute return the same thing.
+  llvm::SmallString<128> InitialDirectory;
+  std::error_code EC = llvm::sys::fs::current_path(InitialDirectory);
+  EXPECT_EQ(EC.value(), 0);
+  FS.setCurrentWorkingDirectory(InitialDirectory);
+
   // Test 1: format file in the same directory.
   ASSERT_TRUE(
   FS.addFile("/a/.clang-format", 0,
@@ -10938,8 +10942,6 @@
   ASSERT_EQ(Style3, getGoogleStyle());
 }
 
-#endif // _MSC_VER
-
 TEST_F(ReplacementTest, FormatCodeAfterReplacements) {
   // Column limit is 20.
   std::string Code = "Type *a =\n"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27440: clang-format-vsix: fail when clang-format outputs to stderr

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D27440#626415, @ioeric wrote:

> `llvm::ErrorOr` carries `std::error_code`. If you want richer information 
> (e.g. error_code + error message), `llvm::Expcted` and `llvm::Error` are 
> your friends.
>
> FYI, if you only need error_code + error_message in the returned error, there 
> is also `llvm::StringError`. And if you want to carry even more information 
> in the errors, you can implement `llvm::ErrorInfo`, which is what we are 
> doing in libTooling replacements library: 
> https://github.com/llvm-mirror/clang/blob/master/include/clang/Tooling/Core/Replacement.h#L150


Thanks, I'll check these out! Btw, I noticed that the clang-format tests are 
non-Windows due to path assumptions. Is this a lost cause, or just something no 
one's bothered to look into yet?


https://reviews.llvm.org/D27440



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27440: clang-format-vsix: fail when clang-format outputs to stderr

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D27440#624917, @djasper wrote:

> Yes.. return non-zero seems right. This is an error condition.


Hi @djasper ,

I started looking into making the changes to clang-format to have it return an 
error code when it's unable to parse the .clang-format file, but I'm not quite 
sure what the best approach is. The function that needs modifying is getStyle 
. 
There are multiple places in there where it outputs to stderr (llvm::errs()) 
when something goes wrong, like here 
.

So here's what I'm thinking in terms of solutions when an error occurs in 
getStyle:

1. Throw an exception. This is unidiomatic in clang-format, and isn't something 
I'm particularly fond of, but it means not modifying the return type, and not 
having to modify the tests since they assume the green path.

2. Return an llvm::ErrorOr. Of course, this changes the signature, which means 
changing the few places that call getStyle. From what I can tell, it's only 
being called in a couple of places, and in the tests, so perhaps it's not 
terrible.

3. Return an llvm::Optional. This is similar to ErrorOr, except that it may 
allow us to codify the fallback behaviour on the outside of this call. What I 
mean is that with Optional, we wouldn't have to pass in the fallback style, but 
rather, the function could return an error when the input style isn't found or 
parsed correctly, etc., and the calling code can decide what to do with the 
error: either stop right there (return an error code from main), or it can try 
to get the fallback style. Something like:



  // The case where we don't care about errors and want to use a fallback style:
  FormatStyle fallbackStyle = getLLVMStyle();
  FormatStyle formatStyle = getStyle("", fileName).getValueOr(fallbackStyle);
  
  
  // The case where we do care about errors
  auto maybeFormatStyle = getStyle("", fileName);
  if (!maybeFormatStyle)
  return 0;
  
  FormatStyle formatStyle = maybeFormatStyle.getValue();

Obviously this third option would change the most code, but maybe it makes more 
sense for getStyle to not have this notion of fallback style within it, as it 
effectively hides errors.

Would love to hear your thoughts.


https://reviews.llvm.org/D27440



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27440: clang-format-vsix: fail when clang-format outputs to stderr

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D27440#624773, @djasper wrote:

> I agree that fallback-style should only be used when there is no 
> .clang-format file. If we find one, and it doesn't parse correctly, we should 
> neither use the fallback style nor scan in higher-level directories (not sure 
> whether we currently do that).


Cool, and do you agree that clang-format should also return non-zero when this 
happens? If so, I'll just abort this change. Perhaps I'll take a look at fixing 
this in clang-format as discussed.


https://reviews.llvm.org/D27440



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27440: clang-format-vsix: fail when clang-format outputs to stderr

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

Hi @djasper, just curious about your opinion on this change, and on the 
conversation following it, specifically regarding whether clang-format should 
really use the fallback style when failing to parse a .clang-format file. 
Thanks!


https://reviews.llvm.org/D27440



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27501: clang-format-vsix: add command to format document

[Antonio Maiorano via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL289910: clang-format-vsix: add command to format document 
(authored by amaiorano).

Changed prior to commit:
  https://reviews.llvm.org/D27501?vs=80533=81701#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27501

Files:
  cfe/trunk/tools/clang-format-vs/.gitignore
  cfe/trunk/tools/clang-format-vs/ClangFormat/ClangFormat.vsct
  cfe/trunk/tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
  cfe/trunk/tools/clang-format-vs/ClangFormat/PkgCmdID.cs
  cfe/trunk/tools/clang-format-vs/README.txt

Index: cfe/trunk/tools/clang-format-vs/README.txt
===
--- cfe/trunk/tools/clang-format-vs/README.txt
+++ cfe/trunk/tools/clang-format-vs/README.txt
@@ -25,3 +25,27 @@
 ClangFormat/source.extension.vsixmanifest. Once the plug-in has been built with
 CMake once, it can be built manually from the ClangFormat.sln solution in Visual
 Studio.
+
+===
+ Debugging
+===
+
+Once you've built the clang_format_vsix project from LLVM.sln at least once,
+open ClangFormat.sln in Visual Studio, then:
+
+- Make sure the "Debug" target is selected
+- Open the ClangFormat project properties
+- Select the Debug tab
+- Set "Start external program:" to where your devenv.exe is installed. Typically
+  it's "C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\IDE\devenv.exe"
+- Set "Command line arguments" to: /rootsuffix Exp
+- You can now set breakpoints if you like
+- Press F5 to build and run with debugger
+
+If all goes well, a new instance of Visual Studio will be launched in a special
+mode where it uses the experimental hive instead of the normal configuration hive.
+By default, when you build a VSIX project in Visual Studio, it auto-registers the
+extension in the experimental hive, allowing you to test it. In the new Visual Studio
+instance, open or create a C++ solution, and you should now see the Clang Format
+entries in the Tool menu. You can test it out, and any breakpoints you set will be
+hit where you can debug as usual.
Index: cfe/trunk/tools/clang-format-vs/.gitignore
===
--- cfe/trunk/tools/clang-format-vs/.gitignore
+++ cfe/trunk/tools/clang-format-vs/.gitignore
@@ -1,5 +1,6 @@
 # Visual Studio files
 .vs/
+*.user
 /packages/
 /ClangFormat/obj/
 /ClangFormat/bin/
Index: cfe/trunk/tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
===
--- cfe/trunk/tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
+++ cfe/trunk/tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
@@ -180,41 +180,86 @@
 var commandService = GetService(typeof(IMenuCommandService)) as OleMenuCommandService;
 if (commandService != null)
 {
-var menuCommandID = new CommandID(GuidList.guidClangFormatCmdSet, (int)PkgCmdIDList.cmdidClangFormat);
-var menuItem = new MenuCommand(MenuItemCallback, menuCommandID);
-commandService.AddCommand(menuItem);
+{
+var menuCommandID = new CommandID(GuidList.guidClangFormatCmdSet, (int)PkgCmdIDList.cmdidClangFormatSelection);
+var menuItem = new MenuCommand(MenuItemCallback, menuCommandID);
+commandService.AddCommand(menuItem);
+}
+
+{
+var menuCommandID = new CommandID(GuidList.guidClangFormatCmdSet, (int)PkgCmdIDList.cmdidClangFormatDocument);
+var menuItem = new MenuCommand(MenuItemCallback, menuCommandID);
+commandService.AddCommand(menuItem);
+}
 }
 }
 #endregion
 
 private void MenuItemCallback(object sender, EventArgs args)
 {
+var mc = sender as System.ComponentModel.Design.MenuCommand;
+if (mc == null)
+return;
+
+switch (mc.CommandID.ID)
+{
+case (int)PkgCmdIDList.cmdidClangFormatSelection:
+FormatSelection();
+break;
+
+case (int)PkgCmdIDList.cmdidClangFormatDocument:
+FormatDocument();
+break;
+}
+}
+
+/// 
+/// Runs clang-format on the current selection
+/// 
+private void FormatSelection()
+{
 IWpfTextView view = GetCurrentView();
 if (view == null)
 // We're not in a text view.
 return;
 string text = view.TextBuffer.CurrentSnapshot.GetText();
 int start = view.Selection.Start.Position.GetContainingLine().Start.Position;
 int end = view.Selection.End.Position.GetContainingLine().End.Position;
 int length = end - start;
+
 // 

[PATCH] D27438: clang-format-vsix: add a date stamp to the VSIX version number to ensure upgradability

[Antonio Maiorano via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rL289909: clang-format-vsix: add a date stamp to the VSIX 
version number to ensure… (authored by amaiorano).

Changed prior to commit:
  https://reviews.llvm.org/D27438?vs=80527=81696#toc

Repository:
  rL LLVM

https://reviews.llvm.org/D27438

Files:
  cfe/trunk/tools/clang-format-vs/CMakeLists.txt


Index: cfe/trunk/tools/clang-format-vs/CMakeLists.txt
===
--- cfe/trunk/tools/clang-format-vs/CMakeLists.txt
+++ cfe/trunk/tools/clang-format-vs/CMakeLists.txt
@@ -11,8 +11,11 @@
   "${CLANG_SOURCE_DIR}/LICENSE.TXT"
   "${CMAKE_CURRENT_SOURCE_DIR}/ClangFormat/license.txt")
 
+  # Build number added to Clang version to ensure that new VSIX can be upgraded
+  string(TIMESTAMP CLANG_FORMAT_VSIX_BUILD %y%m%d%H%M UTC)
+
   if (NOT CLANG_FORMAT_VS_VERSION)
-set(CLANG_FORMAT_VS_VERSION 
"${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}")
+set(CLANG_FORMAT_VS_VERSION 
"${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}.${CLANG_FORMAT_VSIX_BUILD}")
   endif()
 
   configure_file("source.extension.vsixmanifest.in"


Index: cfe/trunk/tools/clang-format-vs/CMakeLists.txt
===
--- cfe/trunk/tools/clang-format-vs/CMakeLists.txt
+++ cfe/trunk/tools/clang-format-vs/CMakeLists.txt
@@ -11,8 +11,11 @@
   "${CLANG_SOURCE_DIR}/LICENSE.TXT"
   "${CMAKE_CURRENT_SOURCE_DIR}/ClangFormat/license.txt")
 
+  # Build number added to Clang version to ensure that new VSIX can be upgraded
+  string(TIMESTAMP CLANG_FORMAT_VSIX_BUILD %y%m%d%H%M UTC)
+
   if (NOT CLANG_FORMAT_VS_VERSION)
-set(CLANG_FORMAT_VS_VERSION "${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}")
+set(CLANG_FORMAT_VS_VERSION "${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}.${CLANG_FORMAT_VSIX_BUILD}")
   endif()
 
   configure_file("source.extension.vsixmanifest.in"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27501: clang-format-vsix: add command to format document

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added inline comments.



Comment at: tools/clang-format-vs/ClangFormat/ClangFormat.vsct:76
+
+  Clang Format Document
+

klimek wrote:
> amaiorano wrote:
> > klimek wrote:
> > > hans wrote:
> > > > amaiorano wrote:
> > > > > hans wrote:
> > > > > > I think File would be better than Document when referring to source 
> > > > > > code.
> > > > > > 
> > > > > > But it seems a little annoying to need two menu alternatives. Could 
> > > > > > we make the regular "Clang Format" option just format the whole 
> > > > > > file if there is currently no selection, or would that be confusing?
> > > > > The reason I chose "Document" is that it mimics the existing 
> > > > > functionality in Visual Studio:
> > > > > {F2661117}
> > > > > 
> > > > > As you can see, "Ctrl+K, Ctrl+F" is bound to format selection by 
> > > > > default; which I assumed is the reason "Ctrl+R, Ctrl+F" was used for 
> > > > > Clang Format (Selection). So along the same lines, I bound "Ctrl + R, 
> > > > > Ctrl + D" to Clang Format Document.
> > > > > 
> > > > > I don't really have a problem with having multiple menu options, 
> > > > > although we could have both of them underneath a "Clang Format" top 
> > > > > menu, with "Format Selection" and "Format Document" as sub menu items.
> > > > > 
> > > > > As for annoyance in menus, I think most developers would reach for 
> > > > > keyboard shortcuts in this case anyway. Furthermore, the next feature 
> > > > > I want to add, building on top of this one, is allowing the user to 
> > > > > enable "Format on Save", which would make this even easier to use.
> > > > I see, I didn't realize they call it documents. And if they also have 
> > > > separate commands for formating selection and formating the whole file, 
> > > > maybe that makes sense for us too.
> > > > 
> > > > Right, I also imagine folks would use this from the keyboard, but there 
> > > > too it's annoying to have two shortcuts for the same thing. But again, 
> > > > if that's how it's generally done in VS...
> > > > 
> > > > I'd like to hear from Manuel on this patch too, though.
> > > Clang-format currently formats the current line / statement if there is 
> > > no selection, and that's how most people I know use it.
> > > Formatting the whole file by default is not what clang-format is 
> > > optimized for, and highly disruptive (eclipse does that, and it's 
> > > annoying).
> > > I'm more wondering why we need a setting for this at all - isn't 
> > > ctrl-a,ctrl-k,ctrl-f equivalent to formatting the whole file?
> > @klimek My own experience, which also seems to match those of others 
> > online, is that clang-format is a fantastic tool for making sure your 
> > code-base follows a strict formatting standard. What many people do is 
> > install Git pre-stage hooks, for instance, to run clang-format across 
> > entire source files, to ensure they're formatted properly. So even if it's 
> > not optimized for that, it works very well :)
> > 
> > Using clang-format to format only the current line doesn't guarantee that 
> > everything will be formatted accordingly. My own experience is that it 
> > sometimes formats the line differently than if you format the entire 
> > document. Or perhaps you end up writing a few lines of code, and formatting 
> > current line doesn't pick up all the lines you've modified. As for 
> > ctrl-a,ctrl-r-ctrl-f, the problem with it is that you lose your cursor 
> > position, as ctrl-a will place it at the end of the file.
> > 
> > You also mention that formatting the entire file is "highly disruptive" 
> > like in eclipse. I'm not sure how eclipse handles that, but so far VS seems 
> > to do a pretty good job. It manages to maintain breakpoint positions quite 
> > well, it manages to keep the cursor in the same place, and it manages to 
> > keep the relative location of the code where the cursor is at visible on 
> > screen. I suspect part of the reason it works well in VS is because 
> > Microsoft made sure it worked for their own format document feature 
> > (ctrl-k,ctrl-d).
> > 
> > As I mentioned earlier, this is a stepping stone towards adding a feature 
> > to "format on save", for which I'd like to offer the ability to choose 
> > "current line/selection" and "document". [[ 
> > https://github.com/Elders/VSE-FormatDocumentOnSave | Here's an example ]] 
> > of an extension that offers this feature, but for VS's built-in formatting.
> Note: with "highly disruptive" I meant only if the default behavior without 
> any selection is to format the whole file, as that basically kills the 
> workflow; that was what Hans suggested.
> 
> I'm not opposed to having an extra key binding if enough folks want it and 
> there is enough benefit - the argument to not move your cursor is a good 
> point.
Thanks! Now I just need to get my svn submit working. Chris Latner created my 
account, etc. but when I tried to submit another accepted change, it 

[PATCH] D27501: clang-format-vsix: add command to format document

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

(NOTE, I forgot to click Submit on the reply to @klimek a couple days ago!)




Comment at: tools/clang-format-vs/ClangFormat/ClangFormat.vsct:76
+
+  Clang Format Document
+

klimek wrote:
> hans wrote:
> > amaiorano wrote:
> > > hans wrote:
> > > > I think File would be better than Document when referring to source 
> > > > code.
> > > > 
> > > > But it seems a little annoying to need two menu alternatives. Could we 
> > > > make the regular "Clang Format" option just format the whole file if 
> > > > there is currently no selection, or would that be confusing?
> > > The reason I chose "Document" is that it mimics the existing 
> > > functionality in Visual Studio:
> > > {F2661117}
> > > 
> > > As you can see, "Ctrl+K, Ctrl+F" is bound to format selection by default; 
> > > which I assumed is the reason "Ctrl+R, Ctrl+F" was used for Clang Format 
> > > (Selection). So along the same lines, I bound "Ctrl + R, Ctrl + D" to 
> > > Clang Format Document.
> > > 
> > > I don't really have a problem with having multiple menu options, although 
> > > we could have both of them underneath a "Clang Format" top menu, with 
> > > "Format Selection" and "Format Document" as sub menu items.
> > > 
> > > As for annoyance in menus, I think most developers would reach for 
> > > keyboard shortcuts in this case anyway. Furthermore, the next feature I 
> > > want to add, building on top of this one, is allowing the user to enable 
> > > "Format on Save", which would make this even easier to use.
> > I see, I didn't realize they call it documents. And if they also have 
> > separate commands for formating selection and formating the whole file, 
> > maybe that makes sense for us too.
> > 
> > Right, I also imagine folks would use this from the keyboard, but there too 
> > it's annoying to have two shortcuts for the same thing. But again, if 
> > that's how it's generally done in VS...
> > 
> > I'd like to hear from Manuel on this patch too, though.
> Clang-format currently formats the current line / statement if there is no 
> selection, and that's how most people I know use it.
> Formatting the whole file by default is not what clang-format is optimized 
> for, and highly disruptive (eclipse does that, and it's annoying).
> I'm more wondering why we need a setting for this at all - isn't 
> ctrl-a,ctrl-k,ctrl-f equivalent to formatting the whole file?
@klimek My own experience, which also seems to match those of others online, is 
that clang-format is a fantastic tool for making sure your code-base follows a 
strict formatting standard. What many people do is install Git pre-stage hooks, 
for instance, to run clang-format across entire source files, to ensure they're 
formatted properly. So even if it's not optimized for that, it works very well 
:)

Using clang-format to format only the current line doesn't guarantee that 
everything will be formatted accordingly. My own experience is that it 
sometimes formats the line differently than if you format the entire document. 
Or perhaps you end up writing a few lines of code, and formatting current line 
doesn't pick up all the lines you've modified. As for ctrl-a,ctrl-r-ctrl-f, the 
problem with it is that you lose your cursor position, as ctrl-a will place it 
at the end of the file.

You also mention that formatting the entire file is "highly disruptive" like in 
eclipse. I'm not sure how eclipse handles that, but so far VS seems to do a 
pretty good job. It manages to maintain breakpoint positions quite well, it 
manages to keep the cursor in the same place, and it manages to keep the 
relative location of the code where the cursor is at visible on screen. I 
suspect part of the reason it works well in VS is because Microsoft made sure 
it worked for their own format document feature (ctrl-k,ctrl-d).

As I mentioned earlier, this is a stepping stone towards adding a feature to 
"format on save", for which I'd like to offer the ability to choose "current 
line/selection" and "document". [[ 
https://github.com/Elders/VSE-FormatDocumentOnSave | Here's an example ]] of an 
extension that offers this feature, but for VS's built-in formatting.


https://reviews.llvm.org/D27501



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27501: clang-format-vsix: add command to format document

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added inline comments.



Comment at: tools/clang-format-vs/ClangFormat/ClangFormat.vsct:76
+
+  Clang Format Document
+

hans wrote:
> I think File would be better than Document when referring to source code.
> 
> But it seems a little annoying to need two menu alternatives. Could we make 
> the regular "Clang Format" option just format the whole file if there is 
> currently no selection, or would that be confusing?
The reason I chose "Document" is that it mimics the existing functionality in 
Visual Studio:
{F2661117}

As you can see, "Ctrl+K, Ctrl+F" is bound to format selection by default; which 
I assumed is the reason "Ctrl+R, Ctrl+F" was used for Clang Format (Selection). 
So along the same lines, I bound "Ctrl + R, Ctrl + D" to Clang Format Document.

I don't really have a problem with having multiple menu options, although we 
could have both of them underneath a "Clang Format" top menu, with "Format 
Selection" and "Format Document" as sub menu items.

As for annoyance in menus, I think most developers would reach for keyboard 
shortcuts in this case anyway. Furthermore, the next feature I want to add, 
building on top of this one, is allowing the user to enable "Format on Save", 
which would make this even easier to use.


https://reviews.llvm.org/D27501



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27501: clang-format-vsix: add command to format document

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano updated this revision to Diff 80533.
amaiorano added a comment.

My first patch accidentally included the changes from 
https://reviews.llvm.org/D27440


https://reviews.llvm.org/D27501

Files:
  tools/clang-format-vs/.gitignore
  tools/clang-format-vs/ClangFormat/ClangFormat.vsct
  tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
  tools/clang-format-vs/ClangFormat/PkgCmdID.cs
  tools/clang-format-vs/README.txt

Index: tools/clang-format-vs/README.txt
===
--- tools/clang-format-vs/README.txt
+++ tools/clang-format-vs/README.txt
@@ -25,3 +25,27 @@
 ClangFormat/source.extension.vsixmanifest. Once the plug-in has been built with
 CMake once, it can be built manually from the ClangFormat.sln solution in Visual
 Studio.
+
+===
+ Debugging
+===
+
+Once you've built the clang_format_vsix project from LLVM.sln at least once,
+open ClangFormat.sln in Visual Studio, then:
+
+- Make sure the "Debug" target is selected
+- Open the ClangFormat project properties
+- Select the Debug tab
+- Set "Start external program:" to where your devenv.exe is installed. Typically
+  it's "C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\IDE\devenv.exe"
+- Set "Command line arguments" to: /rootsuffix Exp
+- You can now set breakpoints if you like
+- Press F5 to build and run with debugger
+
+If all goes well, a new instance of Visual Studio will be launched in a special
+mode where it uses the experimental hive instead of the normal configuration hive.
+By default, when you build a VSIX project in Visual Studio, it auto-registers the
+extension in the experimental hive, allowing you to test it. In the new Visual Studio
+instance, open or create a C++ solution, and you should now see the Clang Format
+entries in the Tool menu. You can test it out, and any breakpoints you set will be
+hit where you can debug as usual.
Index: tools/clang-format-vs/ClangFormat/PkgCmdID.cs
===
--- tools/clang-format-vs/ClangFormat/PkgCmdID.cs
+++ tools/clang-format-vs/ClangFormat/PkgCmdID.cs
@@ -2,6 +2,7 @@
 {
 static class PkgCmdIDList
 {
-public const uint cmdidClangFormat = 0x100;
+public const uint cmdidClangFormatSelection = 0x100;
+public const uint cmdidClangFormatDocument = 0x101;
 };
 }
\ No newline at end of file
Index: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
===
--- tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
+++ tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
@@ -180,41 +180,86 @@
 var commandService = GetService(typeof(IMenuCommandService)) as OleMenuCommandService;
 if (commandService != null)
 {
-var menuCommandID = new CommandID(GuidList.guidClangFormatCmdSet, (int)PkgCmdIDList.cmdidClangFormat);
-var menuItem = new MenuCommand(MenuItemCallback, menuCommandID);
-commandService.AddCommand(menuItem);
+{
+var menuCommandID = new CommandID(GuidList.guidClangFormatCmdSet, (int)PkgCmdIDList.cmdidClangFormatSelection);
+var menuItem = new MenuCommand(MenuItemCallback, menuCommandID);
+commandService.AddCommand(menuItem);
+}
+
+{
+var menuCommandID = new CommandID(GuidList.guidClangFormatCmdSet, (int)PkgCmdIDList.cmdidClangFormatDocument);
+var menuItem = new MenuCommand(MenuItemCallback, menuCommandID);
+commandService.AddCommand(menuItem);
+}
 }
 }
 #endregion
 
 private void MenuItemCallback(object sender, EventArgs args)
 {
+var mc = sender as System.ComponentModel.Design.MenuCommand;
+if (mc == null)
+return;
+
+switch (mc.CommandID.ID)
+{
+case (int)PkgCmdIDList.cmdidClangFormatSelection:
+FormatSelection();
+break;
+
+case (int)PkgCmdIDList.cmdidClangFormatDocument:
+FormatDocument();
+break;
+}
+}
+
+/// 
+/// Runs clang-format on the current selection
+/// 
+private void FormatSelection()
+{
 IWpfTextView view = GetCurrentView();
 if (view == null)
 // We're not in a text view.
 return;
 string text = view.TextBuffer.CurrentSnapshot.GetText();
 int start = view.Selection.Start.Position.GetContainingLine().Start.Position;
 int end = view.Selection.End.Position.GetContainingLine().End.Position;
 int length = end - start;
+
 // clang-format doesn't support formatting a range 

[PATCH] D27501: clang-format-vsix: add command to format document

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano created this revision.
amaiorano added reviewers: hans, zturner, klimek.
amaiorano added a subscriber: cfe-commits.

Bound to Ctrl+R, Ctrl+D by default. Also added section on how to debug the 
extension to the Readme.


https://reviews.llvm.org/D27501

Files:
  tools/clang-format-vs/.gitignore
  tools/clang-format-vs/ClangFormat/ClangFormat.vsct
  tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
  tools/clang-format-vs/ClangFormat/PkgCmdID.cs
  tools/clang-format-vs/README.txt

Index: tools/clang-format-vs/README.txt
===
--- tools/clang-format-vs/README.txt
+++ tools/clang-format-vs/README.txt
@@ -25,3 +25,27 @@
 ClangFormat/source.extension.vsixmanifest. Once the plug-in has been built with
 CMake once, it can be built manually from the ClangFormat.sln solution in Visual
 Studio.
+
+===
+ Debugging
+===
+
+Once you've built the clang_format_vsix project from LLVM.sln at least once,
+open ClangFormat.sln in Visual Studio, then:
+
+- Make sure the "Debug" target is selected
+- Open the ClangFormat project properties
+- Select the Debug tab
+- Set "Start external program:" to where your devenv.exe is installed. Typically
+  it's "C:\Program Files (x86)\Microsoft Visual Studio 14.0\Common7\IDE\devenv.exe"
+- Set "Command line arguments" to: /rootsuffix Exp
+- You can now set breakpoints if you like
+- Press F5 to build and run with debugger
+
+If all goes well, a new instance of Visual Studio will be launched in a special
+mode where it uses the experimental hive instead of the normal configuration hive.
+By default, when you build a VSIX project in Visual Studio, it auto-registers the
+extension in the experimental hive, allowing you to test it. In the new Visual Studio
+instance, open or create a C++ solution, and you should now see the Clang Format
+entries in the Tool menu. You can test it out, and any breakpoints you set will be
+hit where you can debug as usual.
Index: tools/clang-format-vs/ClangFormat/PkgCmdID.cs
===
--- tools/clang-format-vs/ClangFormat/PkgCmdID.cs
+++ tools/clang-format-vs/ClangFormat/PkgCmdID.cs
@@ -2,6 +2,7 @@
 {
 static class PkgCmdIDList
 {
-public const uint cmdidClangFormat = 0x100;
+public const uint cmdidClangFormatSelection = 0x100;
+public const uint cmdidClangFormatDocument = 0x101;
 };
 }
\ No newline at end of file
Index: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
===
--- tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
+++ tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
@@ -180,41 +180,86 @@
 var commandService = GetService(typeof(IMenuCommandService)) as OleMenuCommandService;
 if (commandService != null)
 {
-var menuCommandID = new CommandID(GuidList.guidClangFormatCmdSet, (int)PkgCmdIDList.cmdidClangFormat);
-var menuItem = new MenuCommand(MenuItemCallback, menuCommandID);
-commandService.AddCommand(menuItem);
+{
+var menuCommandID = new CommandID(GuidList.guidClangFormatCmdSet, (int)PkgCmdIDList.cmdidClangFormatSelection);
+var menuItem = new MenuCommand(MenuItemCallback, menuCommandID);
+commandService.AddCommand(menuItem);
+}
+
+{
+var menuCommandID = new CommandID(GuidList.guidClangFormatCmdSet, (int)PkgCmdIDList.cmdidClangFormatDocument);
+var menuItem = new MenuCommand(MenuItemCallback, menuCommandID);
+commandService.AddCommand(menuItem);
+}
 }
 }
 #endregion
 
 private void MenuItemCallback(object sender, EventArgs args)
 {
+var mc = sender as System.ComponentModel.Design.MenuCommand;
+if (mc == null)
+return;
+
+switch (mc.CommandID.ID)
+{
+case (int)PkgCmdIDList.cmdidClangFormatSelection:
+FormatSelection();
+break;
+
+case (int)PkgCmdIDList.cmdidClangFormatDocument:
+FormatDocument();
+break;
+}
+}
+
+/// 
+/// Runs clang-format on the current selection
+/// 
+private void FormatSelection()
+{
 IWpfTextView view = GetCurrentView();
 if (view == null)
 // We're not in a text view.
 return;
 string text = view.TextBuffer.CurrentSnapshot.GetText();
 int start = view.Selection.Start.Position.GetContainingLine().Start.Position;
 int end = view.Selection.End.Position.GetContainingLine().End.Position;
 int length = end - start;
+   

[PATCH] D27438: clang-format-vsix: add a date stamp to the VSIX version number to ensure upgradability

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

> Yes, I think this is still a worthwhile change. Let's add the hour and minute 
> and get it landed.

Alright, added hour and minute. How do we land this change? Is this something I 
can now do myself? Or would you need to commit this for me again?

I have a couple of more interesting changes coming. Perhaps it would make sense 
to grant me commit rights?


https://reviews.llvm.org/D27438



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27438: clang-format-vsix: add a date stamp to the VSIX version number to ensure upgradability

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano updated this revision to Diff 80527.
amaiorano added a comment.

Added hour and minute.


https://reviews.llvm.org/D27438

Files:
  tools/clang-format-vs/CMakeLists.txt


Index: tools/clang-format-vs/CMakeLists.txt
===
--- tools/clang-format-vs/CMakeLists.txt
+++ tools/clang-format-vs/CMakeLists.txt
@@ -11,8 +11,11 @@
   "${CLANG_SOURCE_DIR}/LICENSE.TXT"
   "${CMAKE_CURRENT_SOURCE_DIR}/ClangFormat/license.txt")
 
+  # Build number added to Clang version to ensure that new VSIX can be upgraded
+  string(TIMESTAMP CLANG_FORMAT_VSIX_BUILD %y%m%d%H%M UTC)
+
   if (NOT CLANG_FORMAT_VS_VERSION)
-set(CLANG_FORMAT_VS_VERSION 
"${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}")
+set(CLANG_FORMAT_VS_VERSION 
"${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}.${CLANG_FORMAT_VSIX_BUILD}")
   endif()
 
   configure_file("source.extension.vsixmanifest.in"


Index: tools/clang-format-vs/CMakeLists.txt
===
--- tools/clang-format-vs/CMakeLists.txt
+++ tools/clang-format-vs/CMakeLists.txt
@@ -11,8 +11,11 @@
   "${CLANG_SOURCE_DIR}/LICENSE.TXT"
   "${CMAKE_CURRENT_SOURCE_DIR}/ClangFormat/license.txt")
 
+  # Build number added to Clang version to ensure that new VSIX can be upgraded
+  string(TIMESTAMP CLANG_FORMAT_VSIX_BUILD %y%m%d%H%M UTC)
+
   if (NOT CLANG_FORMAT_VS_VERSION)
-set(CLANG_FORMAT_VS_VERSION "${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}")
+set(CLANG_FORMAT_VS_VERSION "${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}.${CLANG_FORMAT_VSIX_BUILD}")
   endif()
 
   configure_file("source.extension.vsixmanifest.in"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27440: clang-format-vsix: fail when clang-format outputs to stderr

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D27440#614337, @klimek wrote:

> Pondering this a bit - one question is whether we should make clang-format 
> not return 0 if we pass -fallback-style=none (it already has this option) and 
> we can't figure out a style. What do you think?


When you say "it already has this option", do you mean this is what 
fallback-style is set to by default in this extension? Because, in fact, by 
default it's set to LLVM. Personally, I think it should be set to "none" by 
default.

Having said that, to answer your question, personally I think "fallback-style" 
is really an option that only makes sense when "style=file" AND a file is not 
found. If a user has a .clang-format file, and it fails to parse correctly, 
they would likely find it surprising that it would then ignore that file and 
use the fallback style. I would much rather it just fail hard and not use the 
fallback style in this case.


https://reviews.llvm.org/D27440



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27438: clang-format-vsix: add a date stamp to the VSIX version number to ensure upgradability

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

In https://reviews.llvm.org/D27438#614219, @hans wrote:

> When building the snapshots and releases, I usually put the SVN revision 
> number in there to avoid this same problem: 
> http://llvm-cs.pcc.me.uk/utils/release/build_llvm_package.bat#25


Ah, right, I was wondering how that worked. Well, if you don't think it's 
necessary, we don't have to do it. I didn't realize we had this script to build 
the official VSIX. Do you think you'd switch to using this method? Or would you 
prefer that the official releases be bound to the SVN rev number?

> Your patch seems like a good default though. Should we put the hour and 
> minute in there as well, in case one builds multiple versions the same day?

If you still want this change, I can definitely add the hour and minute. Let me 
know!


https://reviews.llvm.org/D27438



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27440: clang-format-vsix: fail when clang-format outputs to stderr

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano added a comment.

With this change, I now get the following message box when my .clang-format has 
an invalid field named "SomeInvalidField":
F2652560: 0.png 

I do have to wonder whether the real bug is the fact that clang-format returns 
a success status (0) even though there's a failure while parsing. Feels odd to 
have it write to stderr but return 0.


https://reviews.llvm.org/D27440



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27440: clang-format-vsix: fail when clang-format outputs to stderr

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano created this revision.
amaiorano added reviewers: klimek, hans, zturner.
amaiorano added a subscriber: cfe-commits.

When clang-format outputs to stderr but returns 0, the extension will format 
the code anyway. This happens, for instance, when there's a syntax error or 
unknown value in a .clang-format file; the result is that the extension 
silently formats using the fallback style without informing the user of the 
problem. This change treats stderr output as an error, making sure it gets 
displayed to the user, and not formatting the code.


https://reviews.llvm.org/D27440

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


Index: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
===
--- tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
+++ tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
@@ -295,12 +295,18 @@
 string output = process.StandardOutput.ReadToEnd();
 // 5. clang-format is done, wait until it is fully shut down.
 process.WaitForExit();
-if (process.ExitCode != 0)
+
+// FIXME: If clang-format writes enough to the standard error 
stream to block,
+// we will never reach this point; instead, read the standard 
error asynchronously.
+string stdErr = process.StandardError.ReadToEnd();
+
+if (process.ExitCode != 0 || stdErr.Length > 0)
 {
-// FIXME: If clang-format writes enough to the standard error 
stream to block,
-// we will never reach this point; instead, read the standard 
error asynchronously.
-throw new Exception(process.StandardError.ReadToEnd());
+throw new Exception(
+(stdErr.Length > 0 ? stdErr + "\n\n" : "") +
+"(Exit Code: " + process.ExitCode + ")");
 }
+
 return output;
 }
 


Index: tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
===
--- tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
+++ tools/clang-format-vs/ClangFormat/ClangFormatPackage.cs
@@ -295,12 +295,18 @@
 string output = process.StandardOutput.ReadToEnd();
 // 5. clang-format is done, wait until it is fully shut down.
 process.WaitForExit();
-if (process.ExitCode != 0)
+
+// FIXME: If clang-format writes enough to the standard error stream to block,
+// we will never reach this point; instead, read the standard error asynchronously.
+string stdErr = process.StandardError.ReadToEnd();
+
+if (process.ExitCode != 0 || stdErr.Length > 0)
 {
-// FIXME: If clang-format writes enough to the standard error stream to block,
-// we will never reach this point; instead, read the standard error asynchronously.
-throw new Exception(process.StandardError.ReadToEnd());
+throw new Exception(
+(stdErr.Length > 0 ? stdErr + "\n\n" : "") +
+"(Exit Code: " + process.ExitCode + ")");
 }
+
 return output;
 }
 
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D27438: clang-format-vsix: add a date stamp to the VSIX version number to ensure upgradability

[Antonio Maiorano via Phabricator via cfe-commits]

amaiorano created this revision.
amaiorano added reviewers: klimek, hans, zturner.
amaiorano added a subscriber: cfe-commits.
Herald added a subscriber: mgorny.

Presently, the version number of the VSIX matches the LLVM version number. 
However, as this number doesn't change often, it means that as we release new 
versions of this VSIX, it will have the same version number, which means users 
must first uninstall the old version before installing the new one. With this 
change, we generate a 4th part to the version number that is a date stamp 
(year, month, day); for example: 4.0.0.161203.


https://reviews.llvm.org/D27438

Files:
  tools/clang-format-vs/CMakeLists.txt


Index: tools/clang-format-vs/CMakeLists.txt
===
--- tools/clang-format-vs/CMakeLists.txt
+++ tools/clang-format-vs/CMakeLists.txt
@@ -11,8 +11,11 @@
   "${CLANG_SOURCE_DIR}/LICENSE.TXT"
   "${CMAKE_CURRENT_SOURCE_DIR}/ClangFormat/license.txt")
 
+  # Build number added to Clang version to ensure that new VSIX can be upgraded
+  string(TIMESTAMP CLANG_FORMAT_VSIX_BUILD %y%m%d UTC)
+
   if (NOT CLANG_FORMAT_VS_VERSION)
-set(CLANG_FORMAT_VS_VERSION 
"${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}")
+set(CLANG_FORMAT_VS_VERSION 
"${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}.${CLANG_FORMAT_VSIX_BUILD}")
   endif()
 
   configure_file("source.extension.vsixmanifest.in"


Index: tools/clang-format-vs/CMakeLists.txt
===
--- tools/clang-format-vs/CMakeLists.txt
+++ tools/clang-format-vs/CMakeLists.txt
@@ -11,8 +11,11 @@
   "${CLANG_SOURCE_DIR}/LICENSE.TXT"
   "${CMAKE_CURRENT_SOURCE_DIR}/ClangFormat/license.txt")
 
+  # Build number added to Clang version to ensure that new VSIX can be upgraded
+  string(TIMESTAMP CLANG_FORMAT_VSIX_BUILD %y%m%d UTC)
+
   if (NOT CLANG_FORMAT_VS_VERSION)
-set(CLANG_FORMAT_VS_VERSION "${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}")
+set(CLANG_FORMAT_VS_VERSION "${LLVM_VERSION_MAJOR}.${LLVM_VERSION_MINOR}.${LLVM_VERSION_PATCH}.${CLANG_FORMAT_VSIX_BUILD}")
   endif()
 
   configure_file("source.extension.vsixmanifest.in"
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
http://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits