[PATCH] D56901: [Support] Fix Windows Command Shell Command line parsing for quoted arguments

MyDeveloperDay via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Jan 18 06:33:48 PST 2019


MyDeveloperDay added a comment.

In D56901#1363040 <https://reviews.llvm.org/D56901#1363040>, @alexfh wrote:

> Where does this problem come from?


https://bugs.llvm.org/show_bug.cgi?id=38471 (in case you didn't see that)

Do we have any documentation that mentions single quotes in the context of command line?

we have bit, but not as much as I originally thought.

http://clang.llvm.org/extra/clang-tidy/  (look for format-style), There are some others in ClangFormat too..

>   I personally doubt that the use of single quotes is common for windows CLI tools. Since single quotes don't have any special meaning on Windows unlike double quotes (used to enclose argument values with spaces in them, IIRC).

Most likely it comes about by the need to put the argument into quotes (double or single) due to the often used wild cards  '-*,-modernize-*'  I notice it always fails if you use it for the --config='....' argument because the YAML fails so as in the bug report the user was using single quote for one argument and double quote for another.

I also think the fact it works on linux shells means those venturing into using clang-tidy on windows are probably copy and pasting examples they might see online which have single quotes in:

https://stackoverflow.com/questions/40723903/how-to-use-clang-tidy-configuration
https://stackoverflow.com/questions/44360961/selectively-disable-clang-tidy-warnings
https://stackoverflow.com/questions/36604325/cppcheck-vs-clang-tidy-explict-constructor-initializer-list

> I believe, the confusion the user had could be addressed by clang-tidy being a bit stricter about the set of characters it accepts in the -checks= option and issue a warning/error in case it sees something weird.

To be honest @alexfh, I feel like I'm using a sledgehammer to crack a nut here!... this isn't exactly the "2nd" commit I wanted to be making given all the tools it could affect, I have also no connection with the original bug, I was just trying to be a good newbie and go fix something small from the bug list as a means of getting more experience.. I may have chosen unwisely ;-)

So I think I like your idea of moving extra error checking up into clang-tidy instead, This might help a little because it will prevent clang-tidy from silently failing but also will have the desired effect, and hopefully won't break the world.

So I let me abandon this review and resubmit something smaller to the clang-tidy guys like @JonasToth and I'll reference this review when I do, so lets not waste any more time on it here if that ok with you guys.

but thanks of your time, and hey I learnt a lot!


Repository:
  rL LLVM

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D56901/new/

https://reviews.llvm.org/D56901





More information about the llvm-commits mailing list