parse arguments in tasks.json #7895
Conversation
Extension/src/common.ts
Outdated
| // Quote a file path if it includes space. | ||
| export function quoteFilePaths(path: string): string { | ||
| // Check if the path is not already quoted. | ||
| if (!/".*?"/.test(path) && !/'.*?'/.test(path)) { |
There was a problem hiding this comment.
- Linux: Cannot use
pkg-configarguments intask.json#7891 relates to use of backtick quoting (which is only supported on mac and linux), but I don't see any handling of backtick quoting here. - It looks like you are using a pattern to check if the entire path is surrounded by quotes. Quoted portions may be contained anywhere within an argument. If there is already quoting within an arg, placing quotes around the whole arg would requiring escaping those internal quotes. Beware if detecting internal quotes within an arg, as they should not be considered as quoting if escaped (
\") - It's unnecessary to detect if the arg contains a path, as args that contain paths are no different from any other arg contents with regards to whether or not they require quoting. i.e. macros/defines may have spaces. (So, this function should be something like
quoteArgument). Since it's perfectly acceptable to quote an argument that does not contain spaces, consider quoting ALL arguments (if not already fully quoted), and escaping any quoting within it. That might be easier than detecting if quoting is necessary. Backticks might be different - I suspect that must always fully quote an arg and cannot be escaped. - Single-quotes are supported only on Linux. They behave as literals on Windows, so shouldn't be checked for on Windows.
There was a problem hiding this comment.
based on our previous discussion:
Assumption: Backtick quoting must be around the entire argument. (Whereas single-quote and double-quote quoting can start in the middle of an argument. For example: -DTEST="TEST TEST", or -DTEST='TEST TEST' on Linux/Mac
The algorithm:
- Trim the arg to remove leading and trailing spaces.
- (On Linux/Mac) If completely enclosed within backtick quoting, leave it alone.
- Check if the string contains any escaped double-quote characters. If so, leave the arg alone, as the user is doing the quoting themselves. Adding additional quoting would change the behavior. (i.e. -DTEST="TEST TEST" is different from "-DTEST="TEST TEST"")
- On Linux or Mac, check if the string contains any unescaped single-quote characters. If so, leave the arg alone. (same reason as above).
- Scan for any spaces not preceded by a slash. Only if there is an un-escaped space is quoting needed.
- If any un-escaped space is found, remove the "" from any escaped spaces ("\ ", in case there were some as they will no longer be valid if quoted), and put double-quotes around the whole thing.
elahehrashedi
force-pushed
the
elrashed/args
branch
from
03f4d09
to
867378f
Compare
elahehrashedi
force-pushed
the
elrashed/args
branch
from
c1d09e3
to
78638be
Compare
|
What do you mean by "@sean-mcmanus the part related to showing errors and warnings might need some modification."? Can you test the scenarios to make sure they all work okay still? |
|
@sean-mcmanus one scenario is not working fine. In a test project, GCC is throwing an error, but |
Can you change the code to correctly detect the error? |
Extension/src/common.ts
Outdated
| export function normalizeArg(arg: string): string { | ||
| arg = arg.trimLeft().trimRight(); | ||
| // Check if the arg is enclosed in backtick, | ||
| // or includes escaped double-quotes, |
There was a problem hiding this comment.
This is a really hard problem, actually... I think we really need a test case for this, so we can plug in various different scenarios to ensure they are supported.
Also, contrary to what I may have said (wrong) in a previous conversation, I think you want to check for any "unescaped" double-quotes here. We should ignore any arg with un-escaped quotes in it, as adding quotes around that (even if escaping them internally) changes the arg... For example:
$ g++ -E -dD -xc++ -DTESTDEFINE="asd asd" /dev/null | grep TESTDEFINE
#define TESTDEFINE asd asd
would become:
$ g++ -E -dD -xc++ "-DTESTDEFINE=\"asd asd\"" /dev/null | grep TESTDEFINE
#define TESTDEFINE "asd asd"
|
@elahehrashedi Is this ready for code review now? |
bug fix: #7891
also partially related to: #6773
@sean-mcmanus the part related to showing errors and warnings might need some modification.