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

Clangd configuration file (.clangd) manager #44

Merged
merged 1 commit into from
Apr 26, 2023

Conversation

ghentschke
Copy link
Contributor

Generates or updates the CompilationDatabase entry in the .clangd yaml file.

@ghentschke ghentschke force-pushed the create-clangd-config branch from 364ae09 to fdb734d Compare April 6, 2023 05:48
@ghentschke ghentschke requested a review from jonahgraham April 6, 2023 08:56
@ghentschke
Copy link
Contributor Author

@jonahgraham the method org.eclipse.cdt.lsp.editor.ui.clangd.ClangdConfigurationManager.setCompilationDatabase(String, String) should be called when the build configuration changes.

I've a open questions:
Does it make sense to use IFile instead of java File objects? The benefit of IFile is, that the IResourceChangeListener will be triggered when the .clangd file will be modfied.

@ghentschke
Copy link
Contributor Author

Fix for #42

@ghentschke ghentschke force-pushed the create-clangd-config branch from fdb734d to 17ad55b Compare April 6, 2023 13:40
@ghentschke
Copy link
Contributor Author

@jonahgraham can you please review if this fits your needs for managed projects?

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.

This LGTM - do you run on Windows? If so and it handles the workspace_loc .toOSString issue then this is good to go. Otherwise some additional work may be needed.

IProject project = event.getProject();
if (project != null && LspEditorPreferencesTester.preferLspEditor(project)) {
ICConfigurationDescription newConfig = newCProjectDecription.getDefaultSettingConfiguration();
var cwdBuilder = newConfig.getBuildSetting().getBuilderCWD();
Copy link
Member

Choose a reason for hiding this comment

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

Its too bad we can't just call org.eclipse.cdt.managedbuilder.makegen.IManagedBuilderMakefileGenerator.getBuildWorkingDir() directly, having to do the variable resolve and then split below is an unfortunate complication.

IProject project = event.getProject();
if (project != null && LspEditorPreferencesTester.preferLspEditor(project)) {
ICConfigurationDescription newConfig = newCProjectDecription.getDefaultSettingConfiguration();
var cwdBuilder = newConfig.getBuildSetting().getBuilderCWD();
Copy link
Member

Choose a reason for hiding this comment

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

Its too bad we can't just call org.eclipse.cdt.managedbuilder.makegen.IManagedBuilderMakefileGenerator.getBuildWorkingDir() directly, having to do the variable resolve and then split below is an unfortunate complication.

*/
@SuppressWarnings("unchecked")
public static void setCompilationDatabase(String configFileDirectory, String databasePath) throws IOException {
var configFile = new File(configFileDirectory, CLANGD_CONFIG_FILE_NAME);
Copy link
Member

Choose a reason for hiding this comment

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

A refresh of configFile at the end of this method is needed (or write to an IFile instead of a java.io.File)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right. I'll use an IFile instead.

private static final String EXPANDED_CDB_SETTING = "CompileFlags: {Add: -ferror-limit=500, CompilationDatabase: %s, Compiler: g++}\nDiagnostics:\n ClangTidy: {Add: modernize*, Remove: modernize-use-trailing-return-type}\n";
private static final String DEFAULT_CDB_SETTING = "CompileFlags: {CompilationDatabase: %s}";
private static final String MODIFIED_DEFAULT_CDB_SETTING = DEFAULT_CDB_SETTING + "\n";
private static final String INVALID_YAML_SYNTAX_CONTAINS_TAB = "CompileFlags:\n\tCompilationDatabase: %s";
Copy link
Member

Choose a reason for hiding this comment

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

The .clangd in CDT I added recently had tabs before the comments. clangd seems fine with it, but I have fixed it in CDT in eclipse-cdt/cdt#382

TBH - not sure how tab got in there in the first place, but I suspect the default text editor in Eclipse with default settings. Maybe we should add .clangd to the set of yml files, or contribute our own language mapping

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH - not sure how tab got in there in the first place, but I suspect the default text editor in Eclipse with default settings. Maybe we should add .clangd to the set of yml files, or contribute our own language mappin

Yepp, that's s a good point

Generates or updates the CompilationDatabase entry in the .clangd yaml
file depending on the active build configuration.
@ghentschke ghentschke force-pushed the create-clangd-config branch from 17ad55b to 219921d Compare April 26, 2023 13:18
@ghentschke
Copy link
Contributor Author

@jonahgraham I just pushed a new commit. Using IFile instead of file. The user will be informed if the yaml file syntax is errornous:
image

@jonahgraham
Copy link
Member

The error popup looks good to me.

@ghentschke
Copy link
Contributor Author

@jonahgraham can you please merge this PR?

@jonahgraham jonahgraham merged commit d791a1d into eclipse-cdt:master Apr 26, 2023
@ghentschke ghentschke deleted the create-clangd-config branch April 26, 2023 14:49
{
@Override
public IStatus runInUIThread(IProgressMonitor monitor) {
ErrorDialog.openError(getActiveShell(), title, errorText, status);
Copy link
Member

Choose a reason for hiding this comment

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

Why do we need to wrap ErrorDialog with UIJob?

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.

3 participants