[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] 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] D27440: clang-format-vsix: fail when clang-format outputs to stderr

[Cameron via Phabricator via cfe-commits]

cameron314 added a comment.

>> 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] D27440: clang-format-vsix: fail when clang-format outputs to stderr

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

In https://reviews.llvm.org/D27440#626477, @amaiorano wrote:

> 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?


No one's bothered looking into it 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#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

[Eric Liu via Phabricator via cfe-commits]

ioeric added a comment.

`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


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

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a reviewer: ioeric.
klimek added a comment.

+eric, who has some experience llvm::Error'ing our interfaces :)
llvm::ErrorOr seems like the right approach here?


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

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

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


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

[Daniel Jasper via Phabricator via cfe-commits]

djasper added a comment.

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


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] D27440: clang-format-vsix: fail when clang-format outputs to stderr

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a reviewer: djasper.
klimek added a comment.

Adding djasper, who had brought forward the idea.


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#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] D27440: clang-format-vsix: fail when clang-format outputs to stderr

[Manuel Klimek via Phabricator via cfe-commits]

klimek added a comment.

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?


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.

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