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

associate *.c++ & *.h++ files with c++ #104339

Merged
merged 5 commits into from Sep 7, 2020
Merged

associate *.c++ & *.h++ files with c++ #104339

merged 5 commits into from Sep 7, 2020

Conversation

therloux
Copy link
Contributor

@therloux therloux commented Aug 9, 2020

This PR fixes #104320

i had to update manualy a .iss file (not so sure i should have -- maybe there's a script that does that automatically à la update-icon-theme?) so appropriate registry keys could be treated the same as .cpp & .h/.hpp files on windows. i also added ".hpp" to the cpp extension so it can be recognised as a c++ file inside vscode. lastly i changed a regex in theme-seti's update-icon-theme.js script file because, even though seti-ui recognised .h++, the script ignored it. that's why i also had to update the icon themes.

i hope those were all of the changes i had to make. maybe you should also add more file extensions. i know there's a lot of them but you should at least add those that are recognised by most compilers by default, so there'd be better compatibility between vscode and c++ compilers (eg. gcc automatically recognises .cp and .hp as c++ files, but vscode doesn't).

@ghost
Copy link

ghost commented Aug 9, 2020

CLA assistant check
All CLA requirements met.

@aeschli
Copy link
Contributor

aeschli commented Aug 12, 2020

@amazingcaio Thanks a lot! Can you remove the seti-font update? I'd rather do that separately. The seti update-script change is fine.

The changes for the language is also fine.

@joaomoreno Do you approve the code.iss changes?

@aeschli aeschli added this to the August 2020 milestone Aug 12, 2020
@therloux
Copy link
Contributor Author

@aeschli done. as you can see, i reverted the update but then i manually edited vs-seti-icon-theme.json to include .h++ files because i think it's necessary for this pull request. i hope it's okay, but if you don't want me to touch that file at all, i can remove those changes.

@aeschli
Copy link
Contributor

aeschli commented Aug 13, 2020

Better remove the vs-seti-icon-theme.json change as well. I'll soon regenerate the seti theme so you will get that change.

@therloux
Copy link
Contributor Author

@aeschli sure :) sorry for not doing it when you first asked.

@therloux
Copy link
Contributor Author

apparently i missed a file. now macos is supposed to recognise *.c++ and *.h++ just like *.cpp and *.hpp

i also noticed there's one other file that should change, extensions/search-result/syntaxes/generateTMLanguage.js (for syntax highlighting in search result tabs), but i'm confused, as all extensions aren't included in the file (e.g. "cxx", "hxx", and "hh" are missing), and .hpp is treated as an objective-c++ file. i suppose not including those file extensions was a mistake, and .hpp has objective-c++ syntax highlighting because it's also used in that language, whose syntax encompasses that of c++.

however, following that logic, shouldn't it apply to .h files as well? currently .h files have objective-c syntax highlighting. wouldn't it be better to treat them as objective-c++?

@JacksonKearl, you're the one who created those files. do you mind explaining it, so we could decide what to do? thx

@JacksonKearl
Copy link
Contributor

JacksonKearl commented Aug 18, 2020

@amazingcaio I'd say for this PR adding cxx, hxx, hh, c++, and h++ mapping to ObjC++ makes sense. I don't think we should treat plain .h as C++ because C++ isn't exactly a superset of C, unlike the Objective-*'s (similarly, .c is not treated at C++). Though I will admit that there wasn't terribly much thought put into programming language theory when constructing that mapping, it's really more of a best-effort approach. 🙂

@therloux
Copy link
Contributor Author

therloux commented Aug 18, 2020

@JacksonKearl i see your point. so i suppose the missing extensions should be added based on the extensions already in the file then. should i change that file for this pull request? i could add all of the missing extensions for c++, or maybe add .c++ & .h++ only, as i believe this would be part of this pr (someone else would then have to update that file with the other extensions), or i could not change those files at all, and someone else (maybe you) could add all of those missing file extensions (including .c++ and .h++).

@JacksonKearl
Copy link
Contributor

I'll just add them all and rebuild the grammar, this PR can stay as-is re. search editors.

@aeschli
Copy link
Contributor

aeschli commented Aug 31, 2020

I already pushed the changes for the file association and the seti script for August. Thanks a lot @amazingcaio !
@joaomoreno just returned from vacation and can hopefully review the rest soon.

@joaomoreno joaomoreno merged commit d93e07f into microsoft:master Sep 7, 2020
@joaomoreno
Copy link
Member

Thanks!

@github-actions github-actions bot locked and limited conversation to collaborators Oct 22, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

add file associations for ".c++" and ".h++" file extensions
5 participants