microsoft / vscode-cpptools

Official repository for the Microsoft C/C++ extension for VS Code.
Other
5.53k stars 1.55k forks source link

Default clang-format is bundled version not system version #6294

Open globalhuman opened 4 years ago

globalhuman commented 4 years ago

Type: LanguageService

Describe the bug

When using clang-format to format a document with no path set for the field "C_Cpp.clang_format_path" the bundled version is used instead of the system version.

"The full path of the clang-format executable. If not specified, and clang-format is available in the environment path, that is used. If not found in the environment path, a copy of clang-format bundled with the extension will be used."

The only way I can tell this is clang-format-10 reorders groups by default where clang-format-6(18.04 ubuntu default) preserves the group by default. This is checked by specify the path to clang-format and observing the changes.

~/.vscode/extensions/ms-vscode.cpptools-1.0.1/LLVM/bin/clang-format --version
clang-format version 10.0.0 (https://github.com/llvm/llvm-project d32170dbd5b0d54436537b6b75beaf44324e0c28)
clang-format --version
clang-format version 6.0.0-1ubuntu2 (tags/RELEASE_600/final)
Colengms commented 4 years ago

I believe this is by design. We are not attempting to match the version of clang-format with the version of LLVM or clang that is installed on the system. Formatting is not dependent on compiler version. Rather, we're trying to provide the most recent version clang-format, so as to include all bug fixes. If you would like to use a specific version of clang-format, it can be specified using the C_Cpp.clang_format_path setting.

globalhuman commented 4 years ago

@Colengms thanks for the reply. Happy for that to be "by design" but the description used to describe the argument indicates otherwise. This text was also included above ^ in my first reply.

This refers to my system installed clang-format

If not specified, and clang-format is available in the environment path

This is the version with the extenison.

a copy of clang-format bundled with the extension will be used.

The full path of the clang-format executable. If not specified, and clang-format is available in the environment path, that is used. If not found in the environment path, a copy of clang-format bundled with the extension will be used.
Colengms commented 4 years ago

@globalhuman Thanks for pointing that out. I believe this functionality was modified to prefer the more recent version of clang-format, if available both on the system as well as bundled, if not explicitly set. It looks like the setting description was not updated to match the new behavior. I will confirm that is the desired functionality, and if so update the setting description, or adjust the behavior. Thanks.

globalhuman commented 4 years ago

It's probably more kind to the users to not change which clang-format is used under the hood (unless you require things from a newer version) since they could be using clang-format from the terminal as a git hook for example which is the system version. Having these all use the same version would be recommended.

sean-mcmanus commented 4 years ago

This behavior was set with PR https://github.com/microsoft/vscode-cpptools/pull/4968 . Seems like a setting could be added to force us to not use the bundled version if another exists.

sean-mcmanus commented 4 years ago

Reasons a new setting would be helpful compared to the workaround of setting clang_format_path: • When the system version changes it’ll get picked up automatically. • The new setting could be settable at a user level (clang_format_path is machine-overridable). • clang_format_path requires users to locate the right one…using “clang-format” doesn’t currently use the PATH to locate the system one.

bobbrow commented 4 years ago

I don't think we need another setting for this. If you just put clang-format as the value of the existing setting, can't we search the PATH and pick the system one instead? (we have code to do this in settings.ts - we'd just need to update the logic of clangFormatPath to allow this way of setting the setting)

sean-mcmanus commented 4 years ago

Yeah, that would be an alternative fix, assuming the machine-overridable isn't an issue...we could potentially change that to "application" as well.

Colengms commented 3 years ago

Using the bundled version of clang-format when it's newer than the system installed version, was a fix for https://github.com/microsoft/vscode-cpptools/issues/4963 . But, that fix seems problematic.

In additional to allowing an absolute path to be specified, the feature should allow either A) Fall back to a system installed version, if present. If not, fall back to the bundled version. or B) Fall back to the bundled version regardless of whether there is a system installed version.

The default (no value specified) should probably be A, as that is currently documented in the description of the setting.

One way of avoiding adding an new setting for B would be to use a clang_format_path value of null. Unfortunately, that would cause the setting to not longer display properly in VS Code's settings UI. (This is an issue we currently have with compilerPath, which is the only other string setting for which null is a valid value).

Another option might be to simply ignore the system installed version if it's too old to accept the command lines args we pass. That would address https://github.com/microsoft/vscode-cpptools/issues/4963 , but it wouldn't enable users to opt-in to our bundled version (other than by setting it as an absolute path, which would break between releases).

Another option might be to add another value to C_Cpp.formatting of Bundled clangFormat. That seems less imperfect than the other solutions.

sean-mcmanus commented 3 years ago

@Colengms The UI issue with compilerPath being null was fixed.