Page MenuHomePhabricator

amaiorano (Antonio Maiorano)
User

Projects

User does not belong to any projects.

User Details

User Since
Dec 5 2016, 5:23 PM (137 w, 11 h)

Recent Activity

May 5 2017

amaiorano abandoned D28419: clang-tools-extra: add gitattributes to disable EOL conversions on checkout of test source files.
May 5 2017, 6:56 AM

Apr 5 2017

amaiorano committed rL299543: clang-format-vsix: Add "Format on Save" feature.
clang-format-vsix: Add "Format on Save" feature
Apr 5 2017, 7:26 AM
amaiorano closed D29221: clang-format-vsix: "format on save" feature by committing rL299543: clang-format-vsix: Add "Format on Save" feature.
Apr 5 2017, 7:26 AM

Mar 30 2017

amaiorano added a comment to D29221: clang-format-vsix: "format on save" feature.
In D29221#713902, @hans 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?

Mar 30 2017, 4:21 AM

Mar 11 2017

amaiorano added a comment to D29221: clang-format-vsix: "format on save" feature.
In 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:

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:
    . 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:


    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.

Mar 11 2017, 11:13 AM

Feb 27 2017

amaiorano added a comment to D29221: clang-format-vsix: "format on save" feature.
In D29221#687425, @hans 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:

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.

Feb 27 2017, 5:56 PM
amaiorano added a comment to D29221: clang-format-vsix: "format on save" feature.
In D29221#687425, @hans 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:

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?

Feb 27 2017, 9:19 AM

Feb 26 2017

amaiorano updated the diff for D29221: clang-format-vsix: "format on save" feature.

Made changes based on @hans 's feedback.

Feb 26 2017, 12:32 PM

Feb 21 2017

amaiorano added inline comments to D29221: clang-format-vsix: "format on save" feature.
Feb 21 2017, 9:23 PM
amaiorano added inline comments to D29221: clang-format-vsix: "format on save" feature.
Feb 21 2017, 9:21 PM

Feb 17 2017

amaiorano added a comment to D29221: clang-format-vsix: "format on save" feature.

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

Feb 17 2017, 8:23 AM

Feb 10 2017

amaiorano updated the diff for D29221: clang-format-vsix: "format on save" feature.
  • Renamed VsixUtils to Vsix and TypeConverterUtils to TypeConversion
  • Removed format on save mode
Feb 10 2017, 7:37 AM

Feb 7 2017

amaiorano added a comment to D29221: clang-format-vsix: "format on save" feature.

+hans

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

Feb 7 2017, 10:01 AM

Feb 6 2017

amaiorano added a comment to D29221: clang-format-vsix: "format on save" feature.

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 :)

Feb 6 2017, 12:17 PM

Jan 27 2017

amaiorano created D29221: clang-format-vsix: "format on save" feature.
Jan 27 2017, 8:19 AM

Jan 23 2017

amaiorano committed rL292787: clang-format: remove tests that assume no config file will be found as this is….
clang-format: remove tests that assume no config file will be found as this is…
Jan 23 2017, 5:31 AM
amaiorano closed D28983: clang-format: remove tests that assume no config file will be found as this is not always the case by committing rL292787: clang-format: remove tests that assume no config file will be found as this is….
Jan 23 2017, 5:31 AM

Jan 21 2017

amaiorano removed reviewers for D28983: clang-format: remove tests that assume no config file will be found as this is not always the case: uabelho, bjope, dmgreen.
Jan 21 2017, 1:27 PM
amaiorano abandoned D27440: clang-format-vsix: fail when clang-format outputs to stderr.

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 (D28081)

Jan 21 2017, 1:25 PM
amaiorano created D28983: clang-format: remove tests that assume no config file will be found as this is not always the case.
Jan 21 2017, 1:19 PM

Jan 20 2017

amaiorano added a comment to D28943: [clang-format] Remove redundant test in style-on-command-line.cpp.

@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".

Jan 20 2017, 6:22 AM
amaiorano added a comment to D28943: [clang-format] Remove redundant test in style-on-command-line.cpp.

@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 :)

Jan 20 2017, 5:13 AM

Jan 19 2017

amaiorano committed rL292562: clang-format: fix fallback style set to "none" not always formatting.
clang-format: fix fallback style set to "none" not always formatting
Jan 19 2017, 5:33 PM
amaiorano closed D28844: clang-format: fix fallback style set to "none" not formatting by committing rL292562: clang-format: fix fallback style set to "none" not always formatting.
Jan 19 2017, 5:33 PM

Jan 17 2017

amaiorano added inline comments to D28844: clang-format: fix fallback style set to "none" not formatting.
Jan 17 2017, 7:52 PM
amaiorano created D28844: clang-format: fix fallback style set to "none" not formatting.
Jan 17 2017, 7:47 PM

Jan 16 2017

amaiorano committed rL292175: Update tools to use new getStyle API.
Update tools to use new getStyle API
Jan 16 2017, 4:24 PM
amaiorano closed D28315: Update tools to use new getStyle API by committing rL292175: Update tools to use new getStyle API.
Jan 16 2017, 4:24 PM
amaiorano committed rL292174: clang-format: Make GetStyle return Expected<FormatStyle> instead of FormatStyle.
clang-format: Make GetStyle return Expected<FormatStyle> instead of FormatStyle
Jan 16 2017, 4:23 PM
amaiorano closed D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle by committing rL292174: clang-format: Make GetStyle return Expected<FormatStyle> instead of FormatStyle.
Jan 16 2017, 4:23 PM
amaiorano added a comment to D28315: Update tools to use new getStyle API.

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 D28419? Or the previous issue has been somehow resolved on Windows?

Jan 16 2017, 5:12 AM
amaiorano added a comment to D28315: Update tools to use new getStyle API.

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.

Jan 16 2017, 3:39 AM
amaiorano updated the diff for D28315: Update tools to use new getStyle API.

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

Jan 16 2017, 3:36 AM
amaiorano updated the diff for D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .

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

Jan 16 2017, 3:34 AM

Jan 15 2017

amaiorano added inline comments to D28315: Update tools to use new getStyle API.
Jan 15 2017, 1:51 PM

Jan 14 2017

amaiorano added inline comments to D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .
Jan 14 2017, 10:08 PM

Jan 11 2017

amaiorano added inline comments to D28315: Update tools to use new getStyle API.
Jan 11 2017, 5:31 AM
amaiorano updated the diff for D28315: Update tools to use new getStyle API.

Fixed code in ClangApplyReplacementsMain.cpp so that we only handle the Expected<Style> when DoFormat is true. Note that I'm using the unidiomatic FormatStyleOrError variable name here, but that's because we only want to set the FormatStyle variable in the parent scope when DoFormat is true.

Jan 11 2017, 4:01 AM
amaiorano updated the diff for D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .

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

Jan 11 2017, 3:58 AM

Jan 10 2017

amaiorano added a comment to D28419: clang-tools-extra: add gitattributes to disable EOL conversions on checkout of test source files.

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?

Jan 10 2017, 11:43 AM

Jan 9 2017

amaiorano added a comment to D28315: Update tools to use new getStyle API.

I ran ninja check-all with D28081 and 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<T> 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" "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"

--

********************
Testing Time: 0.22s
  Expected Passes    : 1

Would you mind running this same command on Linux (with -a -v) and seeing what's different? I don't see why this test would run differently on Windows.

Having said that, I believe I can see one place where I could get Expected<T> must be checked before access or destruction.: in ClangApplyReplacementsMain.cpp, I modified the code to:

llvm::Expected<format::FormatStyle> FormatStyle = format::getNoStyle();
if (DoFormat) {
  FormatStyle = format::getStyle(FormatStyleOpt, FormatStyleConfig, "LLVM");
  if (!FormatStyle) {
    llvm::errs() << llvm::toString(FormatStyle.takeError()) << "\n";
    return 1;
  }
}

If DoFormat is false, then FormatStyle will not be checked. I can fix this, but I would love to get this actual error out of the test myself.

Jan 9 2017, 10:51 PM
amaiorano added a comment to D28315: Update tools to use new getStyle API.

I ran ninja check-all with D28081 and D28315 (in Linux), and some lit tests failed, namely:

Clang :: Format/style-on-command-line.cpp
Jan 9 2017, 9:41 PM
amaiorano added a comment to D28315: Update tools to use new getStyle API.

I ran ninja check-all with D28081 and 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<T> must be checked before access or destruction.

Could you double check? Thanks!

Jan 9 2017, 8:42 AM
amaiorano added a comment to D28419: clang-tools-extra: add gitattributes to disable EOL conversions on checkout of test source files.

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.

Jan 9 2017, 7:53 AM

Jan 6 2017

amaiorano retitled D28419: clang-tools-extra: add gitattributes to disable EOL conversions on checkout of test source files from to clang-tools-extra: add gitattributes to disable EOL conversions on checkout of test source files.
Jan 6 2017, 3:51 PM
amaiorano added a comment to D28315: Update tools to use new getStyle API.

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

Jan 6 2017, 2:24 PM
amaiorano added a comment to D28315: Update tools to use new getStyle API.

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.

Jan 6 2017, 8:41 AM

Jan 5 2017

amaiorano added a comment to D28315: Update tools to use new getStyle API.

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.

Jan 5 2017, 6:25 AM

Jan 4 2017

amaiorano added a comment to D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .

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

Jan 4 2017, 1:47 PM
amaiorano added a comment to D28315: Update tools to use new getStyle API.

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?

Jan 4 2017, 1:46 PM
amaiorano retitled D28315: Update tools to use new getStyle API from to Update tools to use new getStyle API.
Jan 4 2017, 1:45 PM

Jan 3 2017

amaiorano added a comment to D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .

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?

Jan 3 2017, 8:41 PM
amaiorano updated the diff for D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .

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

Jan 3 2017, 8:41 AM
amaiorano added a comment to D27440: clang-format-vsix: fail when clang-format outputs to stderr.

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.

Jan 3 2017, 8:16 AM

Jan 2 2017

amaiorano updated the diff for D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .

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 :)

Jan 2 2017, 8:17 PM
amaiorano added inline comments to D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .
Jan 2 2017, 7:54 PM
amaiorano added a comment to D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .

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!

Jan 2 2017, 3:31 PM
amaiorano updated the diff for D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .

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

Jan 2 2017, 1:01 PM

Jan 1 2017

amaiorano added inline comments to D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .
Jan 1 2017, 10:26 AM

Dec 31 2016

amaiorano added a comment to D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .

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.

Dec 31 2016, 8:35 AM

Dec 29 2016

amaiorano updated the diff for D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .

Updated with the changes @ioeric suggested.

Dec 29 2016, 4:56 PM

Dec 28 2016

amaiorano added inline comments to D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .
Dec 28 2016, 11:47 AM

Dec 27 2016

amaiorano added a comment to D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .

Thank you, @ioeric for your feedback!

Dec 27 2016, 12:38 PM

Dec 24 2016

amaiorano added a comment to D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .

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).".

Dec 24 2016, 9:55 AM

Dec 23 2016

amaiorano added inline comments to D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .
Dec 23 2016, 9:40 PM
amaiorano added reviewers for D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle : cfe-commits, ioeric, djasper, hans, klimek.
Dec 23 2016, 2:08 PM
amaiorano added a comment to D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle .

This change works, and passes all tests; however, I have a bunch of questions, which you'll find in the diffs. I look forward to your feedback. Thanks!

Dec 23 2016, 2:06 PM
amaiorano retitled D28081: Make GetStyle return Expected<FormatStyle> instead of FormatStyle from to Make GetStyle return Expected<FormatStyle> instead of FormatStyle .
Dec 23 2016, 1:42 PM

Dec 21 2016

amaiorano committed rL290319: Make FormatStyle.GetStyleOfFile test work on MSVC.
Make FormatStyle.GetStyleOfFile test work on MSVC
Dec 21 2016, 9:20 PM
amaiorano closed D27971: Make FormatStyle.GetStyleOfFile test work on MSVC by committing rL290319: Make FormatStyle.GetStyleOfFile test work on MSVC.
Dec 21 2016, 9:20 PM
amaiorano added inline comments to D27971: Make FormatStyle.GetStyleOfFile test work on MSVC .
Dec 21 2016, 3:44 AM

Dec 20 2016

amaiorano updated the diff for D27971: Make FormatStyle.GetStyleOfFile test work on MSVC .

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

Dec 20 2016, 5:46 PM
amaiorano committed rL290224: Improve natvis for llvm::SmallString so that it correctly displays only the….
Improve natvis for llvm::SmallString so that it correctly displays only the…
Dec 20 2016, 5:16 PM
amaiorano closed D27972: Improve natvis for llvm::SmallString so that it correctly displays only the valid portion of the string by committing rL290224: Improve natvis for llvm::SmallString so that it correctly displays only the….
Dec 20 2016, 5:16 PM
amaiorano added a comment to D27971: Make FormatStyle.GetStyleOfFile test work on MSVC .

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.

Dec 20 2016, 7:00 AM

Dec 19 2016

amaiorano added a comment to D27972: Improve natvis for llvm::SmallString so that it correctly displays only the valid portion of the string .

Updated screenshot:

Dec 19 2016, 10:05 PM
amaiorano updated the diff for D27972: Improve natvis for llvm::SmallString so that it correctly displays only the valid portion of the string .

Added "na" format specifier to not show the pointer address to the string, just like the original visualizer did. Updated screenshot of what it looks like forthcoming...

Dec 19 2016, 10:05 PM
amaiorano added a comment to D27972: Improve natvis for llvm::SmallString so that it correctly displays only the valid portion of the string .

Goes from:


to:

Dec 19 2016, 9:57 PM
amaiorano added reviewers for D27972: Improve natvis for llvm::SmallString so that it correctly displays only the valid portion of the string : llvm-commits, mspertus, zturner.
Dec 19 2016, 9:54 PM
amaiorano retitled D27972: Improve natvis for llvm::SmallString so that it correctly displays only the valid portion of the string from to Improve natvis for llvm::SmallString so that it correctly displays only the valid portion of the string .
Dec 19 2016, 9:52 PM
amaiorano retitled D27971: Make FormatStyle.GetStyleOfFile test work on MSVC from to Make FormatStyle.GetStyleOfFile test work on MSVC .
Dec 19 2016, 9:39 PM
amaiorano added a comment to D27440: clang-format-vsix: fail when clang-format outputs to stderr.

llvm::ErrorOr carries std::error_code. If you want richer information (e.g. error_code + error message), llvm::Expcted<T> 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

Dec 19 2016, 5:50 AM

Dec 18 2016

amaiorano added a comment to D27440: clang-format-vsix: fail when clang-format outputs to stderr.

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

Dec 18 2016, 6:15 AM

Dec 16 2016

amaiorano added a comment to D27440: clang-format-vsix: fail when clang-format outputs to stderr.

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).

Dec 16 2016, 4:53 AM

Dec 15 2016

amaiorano added a comment to D27440: clang-format-vsix: fail when clang-format outputs to stderr.

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!

Dec 15 2016, 8:48 PM
amaiorano added a comment to D27440: clang-format-vsix: fail when clang-format outputs to stderr.

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!

Dec 15 2016, 8:42 PM
amaiorano committed rL289910: clang-format-vsix: add command to format document.
clang-format-vsix: add command to format document
Dec 15 2016, 6:02 PM
amaiorano closed D27501: clang-format-vsix: add command to format document by committing rL289910: clang-format-vsix: add command to format document.
Dec 15 2016, 6:02 PM
amaiorano committed rL289909: clang-format-vsix: add a date stamp to the VSIX version number to ensure….
clang-format-vsix: add a date stamp to the VSIX version number to ensure…
Dec 15 2016, 5:47 PM
amaiorano closed D27438: clang-format-vsix: add a date stamp to the VSIX version number to ensure upgradability by committing rL289909: clang-format-vsix: add a date stamp to the VSIX version number to ensure….
Dec 15 2016, 5:47 PM

Dec 12 2016

amaiorano added inline comments to D27501: clang-format-vsix: add command to format document.
Dec 12 2016, 7:33 AM

Dec 9 2016

amaiorano added a comment to D27501: clang-format-vsix: add command to format document.

(NOTE, I forgot to click Submit on the reply to @klimek yesterday)

Dec 9 2016, 9:39 AM

Dec 7 2016

amaiorano added inline comments to D27501: clang-format-vsix: add command to format document.
Dec 7 2016, 11:46 AM

Dec 6 2016

amaiorano updated the diff for D27501: clang-format-vsix: add command to format document.

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

Dec 6 2016, 7:09 PM
amaiorano retitled D27501: clang-format-vsix: add command to format document from to clang-format-vsix: add command to format document.
Dec 6 2016, 7:02 PM
amaiorano added a comment to D27438: clang-format-vsix: add a date stamp to the VSIX version number to ensure upgradability.

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

Dec 6 2016, 6:32 PM
amaiorano updated the diff for D27438: clang-format-vsix: add a date stamp to the VSIX version number to ensure upgradability.

Added hour and minute.

Dec 6 2016, 6:29 PM
amaiorano added a comment to D27440: clang-format-vsix: fail when clang-format outputs to stderr.

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?

Dec 6 2016, 3:24 AM

Dec 5 2016

amaiorano added a comment to D27438: clang-format-vsix: add a date stamp to the VSIX version number to ensure upgradability.
In 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

Dec 5 2016, 8:01 PM
amaiorano added a comment to D27440: clang-format-vsix: fail when clang-format outputs to stderr.

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

Dec 5 2016, 5:46 PM