Skip to content
This repository has been archived by the owner on Dec 15, 2022. It is now read-only.

Removed .h from C. C is a subset of the superset C++ and does not req… #156

Merged
merged 2 commits into from
Sep 19, 2016

Conversation

hgdsraj
Copy link
Contributor

@hgdsraj hgdsraj commented Aug 14, 2016

Removed .h from C. C is a subset of the superset C++ and does not require .h within it. Atom looks at C first and formats and syntax highlights .h class files like C does and this creates problems. C highlighting will be the same, CPP class highlighting fixed.

…uire .h within it. Atom looks at C first and formats and syntax highlights .h class files like C does and this creates problems. C highlighting will be the same, CPP class highlighting fixed.
@hgdsraj
Copy link
Contributor Author

hgdsraj commented Sep 4, 2016

Any updates here?
Thanks! :)

@winstliu
Copy link
Contributor

winstliu commented Sep 4, 2016

/cc @thomasjo: Would this create any problems by highlighting all C header files as if they were C++, or is the existing problem (highlighting all C++ header files as if they were C) a bigger problem?

@hgdsraj
Copy link
Contributor Author

hgdsraj commented Sep 4, 2016

C++ is a larger problem, the only issue arises if someone names a variable
in C with a C++ keyword such as "class". This is unlikely but not
impossible, in which case a user simply switches his language to C using
CTRL+SHIFT+L. In almost all cases, however, the benefit of this outweighs
the cons. Currently, we attempt to default to the subset (C) whereas the
superset (C++) should be defaulted for .h . Some use .hpp, etc, however in
some coding rulebooks, one uses .h for header files, where .hpp is reserved
for header+implementation combined files.

On Sun, Sep 4, 2016, 12:18 PM Wliu [email protected] wrote:

/cc @thomasjo https://github.com/thomasjo: Would this create any
problems by highlighting all C header files as if they were C++, or is the
existing problem (highlighting all C++ header files as if they were C) a
bigger problem?


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#156 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ARyOJf0oHk9okP01rA4-kwup9Y4Dt9u3ks5qmxmJgaJpZM4Jj_NJ
.

@thomasjo
Copy link
Contributor

thomasjo commented Sep 5, 2016

Just to set the record straight, C is not a subset of C++. That being said, flipping our current defaults, should not pose any major issues. Although it will incorrectly highlight valid C code that uses C++ keywords such as class, private, public, and so on.

The current situation isn't really a major issue since it is trivially solved by

customFileTypes:
  "source.cpp": [
    "h"
  ]

I'm not opposed to the proposed change.

/cc @atom/feedback

@hgdsraj
Copy link
Contributor Author

hgdsraj commented Sep 5, 2016

I totally agree and perhaps should have ascertained that I was not
approaching the set analogy from a rigorously mathematical standpoint.
Thank you for your comments. Those C++ keywords are the main problem,
however, I feel the benefits outweigh the costs as those keywords are
seldom used as variable names.

If there is an alternative fix you suggest that I might be able to
implement let me know!

As it stands, the change should have very little negative for its positive.

Thanks!

On Sun, Sep 4, 2016, 10:37 PM Thomas Johansen [email protected]
wrote:

Just to set the record straight, C is not a subset of C++
http://www.stroustrup.com/bs_faq.html#C-is-subset. That being said,
flipping our current defaults, should not pose any major issues. Although
it will incorrectly highlight valid C code that uses C++ keywords such
as class, private, public, and so on.

The current situation isn't really a major issue since it is trivially
solved by

customFileTypes:
"source.cpp": [
"h"
]

I'm not opposed to the proposed change.

/cc @atom/feedback https://github.com/orgs/atom/teams/feedback


You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
#156 (comment), or mute
the thread
https://github.com/notifications/unsubscribe-auth/ARyOJeNpf422pRJYORujUbYwodryhZymks5qm6qsgaJpZM4Jj_NJ
.

@thomasjo
Copy link
Contributor

thomasjo commented Sep 5, 2016

As it stands, the change should have very little negative for its positive.

Yeah, I second this. There should be no major issues caused by this change, but I'd prefer to get some extra input and 👀 from some other maintainers before merging.

Just to remove any uncertainty, I'm 👍 on this.

@hgdsraj
Copy link
Contributor Author

hgdsraj commented Sep 5, 2016

Thanks for the label, I agree on the extra eyes and input!

@winstliu
Copy link
Contributor

This seems like it's for the best. Merging.

@winstliu winstliu merged commit 40b75cd into atom:master Sep 19, 2016
@hgdsraj
Copy link
Contributor Author

hgdsraj commented Sep 19, 2016

Sounds great, thanks!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants