search

vegastrike / Vega-Strike-Engine-Source

Vega Strike Engine
Other
256 stars 42 forks source link

Enhancement: Uniform Header Guards #784

Closed BenjamenMeyer closed 1 year ago

BenjamenMeyer commented 1 year ago

Thank you for submitting a pull request and becoming a contributor to the Vega Strike Core Engine.

Please answer the following:

Code Changes:

Did some play testing, but not the official validation play test yet

Issues: n/a

Purpose: Apply a uniform header guard format and denote what headers are not using header guards so we can try to get a handle on it.

0.9.x

BenjamenMeyer commented 1 year ago

NOTE: I noticed that we display the engine version when pressing "ALT+M" during flight and the version is out-of-date. See #785

stephengtuggy commented 1 year ago

I went through the first few dozen header files. Looks good overall, other than that one typo I found.

One request I would like to make is that we update the copyleft notice at the top of each of these files also. They should say, Copyright (C) 2001-2023 Daniel Horn, ... , Benjamen Meyer, and other Vega Strike contributors and otherwise follow our standard format for the copyleft block.

As far as I can tell, you are exactly following the relevant sections of the Google C++ Style Guide here, so nice job! Thanks for putting the effort into this.

BenjamenMeyer commented 1 year ago

@stephengtuggy I didn't want to touch copyleft notices in this PR, wanting to save license changes for its own PR.

I part, I wanted to keep this PR clear on what it was changing as it is a big PR (file count wise); and in part I wanted to do another PR with license changes specifically, probably after trying to move out a couple third party files that shouldn't get licensed by us (need to see if they differ at all from their sources or since they got included too) or at least to make them easier to sync with their source and maintain or possibly just pull in from their source as a dependency.

So the license changes and notices will be coming.

stephengtuggy commented 1 year ago

@stephengtuggy I didn't want to touch copyleft notices in this PR, wanting to save license changes for its own PR.

I part, I wanted to keep this PR clear on what it was changing as it is a big PR (file count wise); and in part I wanted to do another PR with license changes specifically, probably after trying to move out a couple third party files that shouldn't get licensed by us (need to see if they differ at all from their sources or since they got included too) or at least to make them easier to sync with their source and maintain or possibly just pull in from their source as a dependency.

So the license changes and notices will be coming.

@BenjamenMeyer To be clear, I wasn't talking about putting the files under a different license. I was talking only about updating the copyright year; adding yourself as a contributor, where you aren't already listed; and adjusting the formatting of the license notice block. Nothing else.

E.g, starting the license block with the filename, which not all our files do yet; putting the Copyright (C) ... two lines below that; and so forth. These are things that we should do on an ongoing basis, every time we touch a file.

And, yes, there are some files that don't yet have a license notice -- or at least, not one that references Vega Strike -- because we haven't figured out yet how they should be licensed. You can certainly leave those alone, for now.

You also don't need to change any files that you haven't already touched as part of the set of changes that is the main point of this PR.

Does that make sense?

BenjamenMeyer commented 1 year ago

@stephengtuggy for the most part my changes would be considered de minimis so could skate the copyright change, but needless to say it's updated. We should update the date formats to be more consistent though.

stephengtuggy commented 1 year ago

Looks good. Thanks. Merging.