Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

c_cpp_properties.json: Support ${workspaceFolder} substitution #741

Open
seanmlyons22 opened this issue Apr 7, 2021 · 4 comments
Open

Comments

@seanmlyons22
Copy link

c_cpp_properties.json supports vscode variables such as ${workspaceFolder}. However, this must be translated to something that LLVM can understand.
I propose a small enhancement to the CCppProperties class where ECC will attempt to replace ${workspaceFolder} with . in the includePath list. According to this issue it seems that assuming that .vscode/c_cpp_properties.json is located in the root folder is a safe assumption.

I don't believe this would impact any users who do not have ${workspaceFolder} in their c_cpp_properties file.

Create minimal example if you can

An example of a json that I would like to add support for:

{
  "version": 4,
  "configurations": [
    {
      "name": "Win32",
      "includePath": [
        "${workspaceFolder}/source",
      ],
      "defines": [
      ],
    },
  ]
}

I would be willing to open a PR for this if acceptable. I tested the following change and it worked for my use case.
If you're interested, I can make some tests and formalize a PR:

        def parse_includes_from_json(content):
            try:
                include_paths = content["configurations"][0]["includePath"]
            except Exception:
                include_paths = []

            # VSCode variable for current working directory
            vsc_ws = '${workspaceFolder}'
            # Attempt to replace VSCode variables if any
            includes = [i.replace(vsc_ws, '.') for i in include_paths]
            includes = [path.expandvars(i) for i in include_paths]
            includes = ["-I{}".format(include) for include in includes]
            return includes
@seanmlyons22 seanmlyons22 changed the title c_cpp_properties.json: Support ${workspaceFolder} substituion c_cpp_properties.json: Support ${workspaceFolder} substitution Apr 7, 2021
@niosus
Copy link
Owner

niosus commented Apr 10, 2021

I was not the one who implemented the support for c_cpp_properties.json, so if you think it can work better, I am happy to have a look at a PR and merge it eventually, but I will not be able to provide much guidance apart from checking how it plays together with the other parts of the code.

@mclayton7
Copy link
Contributor

@niosus @seanmlyons22 I think this sounds reasonable. There should be some tests that'll help verify the implementation, but if you have any questions about the existing implementation, don't hesitate to @ me!

@niosus
Copy link
Owner

niosus commented Apr 18, 2021

Thanks for chiming in @mclayton7, really appreciate this! @seanmlyons22 feel free to implement what you consider makes sense, open a PR and ping both me and @mclayton7 on it. This way we can go forward with this.

@stale
Copy link

stale bot commented Apr 18, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Just comment here to prevent this from happening.

@stale stale bot added the wontfix label Apr 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants