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

JSON-escape strings #4

Merged
merged 5 commits into from
Jun 27, 2024
Merged

JSON-escape strings #4

merged 5 commits into from
Jun 27, 2024

Conversation

d3m3t3r
Copy link
Contributor

@d3m3t3r d3m3t3r commented May 27, 2024

Add JSON-escaping for string values (e.g. arguments and filenames) in the output.

@i-ky i-ky self-assigned this May 28, 2024
src/basset.cpp Outdated Show resolved Hide resolved
src/basset.cpp Outdated Show resolved Hide resolved
src/basset.cpp Outdated Show resolved Hide resolved
@i-ky
Copy link
Owner

i-ky commented May 28, 2024

At the moment to use basset one needs to run a clean build from zero. But it would be nice to add support for partial builds, when intercepted commands are appended to the existing compile_commands.json instead of overwriting it completely. This will require not just writing, but also reading JSON and will likely require a proper JSON library. Escaping can then also be done using JSON library.

I believe this was my thought process when I decided to completely ignore the need to escape strings in JSON. So maybe instead of implementing our own JSON escaping implementation you can help me pick a good JSON library to use?

src/basset.cpp Outdated Show resolved Hide resolved
src/basset.cpp Outdated Show resolved Hide resolved
@d3m3t3r
Copy link
Contributor Author

d3m3t3r commented Jun 21, 2024

I'm trying to implement support for partial builds (with https://github.com/nlohmann/json for JSON handling) and as you recognized it will make this PR obsolete.

BTW, when the new command replaces an existing command(-s?) in the database? Never? Meaning, new commands are always appended except for exact duplicates (e.g. same "filename", "directory" and "arguments")?

@i-ky
Copy link
Owner

i-ky commented Jun 25, 2024

I'm trying to implement support for partial builds (with https://github.com/nlohmann/json for JSON handling) and as you recognized it will make this PR obsolete.

This sounds great! 🤩

BTW, when the new command replaces an existing command(-s?) in the database? Never? Meaning, new commands are always appended except for exact duplicates (e.g. same "filename", "directory" and "arguments")?

This is a very good question. On one hand, clangd and probably other consumers of compile_commands.json only support one compilation command per file, so for them only replacing would make sense. On the other hand, compile_commands.json format itself and probably some consumers (e.g. run-clang-tidy) support multiple compilation commands per file, for them appending will make sense. I think a good enough approach would be to mimic what Bear does.

Actually, speaking of Bear... It is an option to "simply" integrate basset with bear as a low-level intercept mechanism, this way all high level functionality (like support for partial builds) will be automatically provided by bear. basset will focus entirely on intercepting the commands. See the relevant discussion.

@i-ky i-ky merged commit 54f7f72 into i-ky:main Jun 27, 2024
@i-ky
Copy link
Owner

i-ky commented Jun 27, 2024

@d3m3t3r, thank you for your contribution!

@d3m3t3r d3m3t3r deleted the json-escape branch June 28, 2024 12:48
@d3m3t3r
Copy link
Contributor Author

d3m3t3r commented Jul 3, 2024

This is what I come up for the support of incremental update so far: https://github.com/d3m3t3r/basset/tree/incremental-update

@i-ky
Copy link
Owner

i-ky commented Jul 4, 2024

@d3m3t3r, don't be shy! Feel free to create a PR. If it is still "work in progress", you can mark it as draft. In any case it will serve as advertisement of your work and upcoming feature in basset.

@i-ky i-ky mentioned this pull request Jul 6, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants