Skip to content

Conversation

travkin79
Copy link
Contributor

Instead of creating a .clang-format file for each project, CDT LSP now would check if there is a .clang-format file in some of the project's parent folders and use that. This way, multi-project directory structures (multi-project git repos) can offer common settings in their root folder that apply to all contained projects.

@jonahgraham jonahgraham linked an issue Feb 25, 2025 that may be closed by this pull request
Copy link
Member

@jonahgraham jonahgraham left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@travkin79 Thanks for putting this together. I think this is generally the right idea and avoiding creating clang format files in this case is a good idea.

I see this is only a draft at the moment. I did a quick review because I would really like to get this merged by end of the day Wednesday, assuming no objection.

Let me know what I can do to make this happen.

@travkin79
Copy link
Contributor Author

travkin79 commented Feb 26, 2025

Hi @jonahgraham,
Thank you for the quick review. I'll check the points and will try to improve that. The implementation was a first (quick and dirty) version and has to be tested more thoroughly, that's why it is a draft.

Let me know what I can do to make this happen.

I think, I could finish the PR today.

Do you know of any place where the .clang-format file is actually read by CDT LSP? The ClangFormatHandler does only create the file and optionally opens it using LSP4E. I just want to be sure that I've adapted all affected code snippets.

@travkin79 travkin79 marked this pull request as ready for review February 26, 2025 10:32
@jonahgraham
Copy link
Member

Hi @jonahgraham,
Thank you for the quick review. I'll check the points and will try to improve that. The implementation was a first (quick and dirty) version and has to be tested more thoroughly, that's why it is a draft.

👍

Let me know what I can do to make this happen.

I think, I could finish the PR today.

Awesome! Thanks

Do you know of any place where the .clang-format file is actually read by CDT LSP? The ClangFormatHandler does only create the file and optionally opens it using LSP4E. I just want to be sure that I've adapted all affected code snippets.

I don't think it is read by the Java code. Tagging @ghentschke in case she has any additional input

@travkin79
Copy link
Contributor Author

I think, there is still a limitation in the approach: the ClangFormatFileMonitor only listens to changes to .clang-format files in the Eclipse workspace (only for files in the projects' root folders). If there is a .clang-format file in a project's parent folder outside the Eclipse workspace (e.g. in a multi-project git repo), we have no IFile (we have an IFileStore instead) and nobody listens to changes in such a .clang-format file. I'm not sure if there is some kind of IFileStoreListener concept available to cover that case, too.

@jonahgraham
Copy link
Member

I think, there is still a limitation in the approach [...]

Your analysis is correct - the resource change listener is there to validate the clang format file. There are still some wider issues with this and I will add a comment about this case to #400

I don't think this is blocking as having CDT LSP create the clang format file when it shouldn't is a worse error to me than missing some invalid .clang-format settings.

@jonahgraham jonahgraham merged commit ccf1be4 into eclipse-cdt:main Feb 26, 2025
3 checks passed
@jonahgraham
Copy link
Member

I have submitted this now because I would like it in RC1 which I will publish today.

@ghentschke I didn't wait for your review, if you want a change let me know andI can do a respin of RC1, otherwise it will make it into RC2.

@ghentschke
Copy link
Contributor

@jonahgraham I fine. Thank you.

@jonahgraham jonahgraham added this to the 3.0.0 milestone Mar 7, 2025
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.

Do not create .clangd-format file in project if parent has one
3 participants