Fix ${workspaceFolder} being added to browse.path if ${workspaceFolder}/* is used. #13324
Conversation
sean-mcmanus
changed the title
| @@ -998,10 +998,15 @@ export class CppProperties { | |||
| } else if (configuration.includePath) { | |||
| // If the user doesn't set browse.path, copy the includePath over. Make sure ${workspaceFolder} is in there though... | |||
There was a problem hiding this comment.
After my current PR on the native side goes in, I want to remove this section, making it the responsibility of the native side to populate the browse paths with the include paths, if the browse path is undefined. That way, I can ensure any include paths ingested via compiler arguments get into the browse path also (if not explicitly set). Since this would be removed soon (before the next insiders, if possible), maybe this fix isn't needed.
There was a problem hiding this comment.
@Colengms Sure, yeah, it sounds like this won't be needed.
| !!value.match(/^\$\{(workspaceRoot|workspaceFolder)\}(\\\*{0,2}|\/\*{0,2})?$/g)) === -1 | ||
| ) { | ||
| configuration.browse.path.push("${workspaceFolder}"); | ||
| if (this.rootUri) { |
There was a problem hiding this comment.
Do we know if this behavior changed due to a change in vscode itself, or something else?
if it was a change in vscode, did we want to still make it work with (workspaceRoot|workspaceFolder) as well in case the newer extension was run on an older version of vscode?
There was a problem hiding this comment.
It needs to continue to work with ${workspaceFolder}/** because that's the default entry for includePath.
There was a problem hiding this comment.
@fearthecowboy @bobbrow The change is that this the ${workspaceFolder} variable is resolved at this point by our own code due to calls to the resolveVariable or whatever it's called.
There was a problem hiding this comment.
resolveVariables does not resolve ${workspaceFolder}. That happens in the native code.
There was a problem hiding this comment.
@bobbrow No, that changed. It resolves it in TypeScript. That's what I saw when I debugged it.
This was working with 1.16.3, but then some change caused the ${workspaceFolder} variable to be resolved before it checked for ${workspaceFolder} at this spot, so the regex match would always fail. This just fixes it so that the previously working case works now.